Conversation
…a where 'd_' defines the dark icon.
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## master #656 +/- ##
=======================================
Coverage 36.05% 36.05%
=======================================
Files 277 277
Lines 34909 34909
=======================================
Hits 12588 12588
Misses 22321 22321
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| /// <param name="skin"></param> | ||
| /// <returns></returns> | ||
| public static Texture2D GetIcon(string iconName, IconSkin skin = IconSkin.Default) | ||
| public static Texture2D GetIcon(string iconName, string withPrefix = "", string withSuffix = "_Light", IconSkin skin = IconSkin.Default) |
There was a problem hiding this comment.
I'm confused, isn't that the old convention? Shouldn't we default to the prefix d_ and not the suffix _Light?
There was a problem hiding this comment.
Also, I would think it's better when adding on top of existing API the new parameters at the end instead of in the middle to avoid causing compilation errors if people upgrade
| const string k_IconPath = "EditableMesh/EditMeshContext"; | ||
| const string k_ComponentMessage = "Use the ProBuilder Edit Mode in the Scene Tools Overlay to edit this Mesh."; | ||
| public static readonly GUIContent helpLabelContentIcon = new GUIContent(IconUtility.GetIcon(k_IconPath)); | ||
| public static readonly GUIContent helpLabelContentIcon = new GUIContent(IconUtility.GetIcon(k_IconPath, "d_", null)); |
There was a problem hiding this comment.
Not enough of a c# expert, but how does it handle concatenating null string? I would play it safe with ""
There was a problem hiding this comment.
My bad, I've set a nullable string when I started to make the changes, before going back to string.Empty in the GetIcon's signature. Will update this. Thanks for catching this!
| wrapU: 1 | ||
| wrapV: 1 | ||
| wrapW: 1 | ||
| nPOTScale: 0 |
There was a problem hiding this comment.
just wondering why the meta files changed so much, especially those values
There was a problem hiding this comment.
I'm going to try to revert the changes on the importer settings. There shouldn't be any change there.
| @@ -26,10 +26,12 @@ static class IconUtility | |||
| /// <param name="iconName"></param> | |||
| /// <param name="skin"></param> | |||
2 participants
DO NOT FORGET TO INCLUDE A CHANGELOG ENTRY
Purpose of this PR
When using the Editor Light theme, some icons pulled are from the Dark theme. The affected icons are reference throught the
[Icon(icon-path)]attribute so they follow the rule where file paths should be named as following:Probuilder handles the loading and the differentiation between both skins for most icon, by using its own
IconUtility.GetIcon()but it follow differents rules:I've updated the method so we can override for the specific case where the targeted icon is also being used with the
IconAttribute. It's the case for one of the icon where it's in an attribute, but also being loaded through the API.Before:
After:
Links
Jira: UUM-133531
Comments to Reviewers
[List known issues, planned work, provide any extra context for your code.]