Icon refactoring #3956
Icon refactoring #3956
Conversation
| @@ -42,9 +41,6 @@ public class Stock | |||
| public static readonly IconId CloseAllDocuments = "md-close-all-documents"; | |||
| public static readonly IconId CloseCombine = "md-close-combine-icon"; | |||
| public static readonly IconId CloseIcon = Gtk.Stock.Close; | |||
| public static readonly IconId ClosedFolder = "md-closed-folder"; | |||
| public static readonly IconId ClosedReferenceFolder = "md-closed-reference-folder"; | |||
| public static readonly IconId ClosedResourceFolder = "md-closed-resource-folder"; | |||
There was a problem hiding this comment.
This is a public API break. I think we need to keep the old names otherwise this will break any third party addins that use these Stock icons.
There was a problem hiding this comment.
@mrward See the last commit.
So we don't break the API.
|
Could you please review this PR? I'd like to know if this is fine before I do the same thing in |
| <StockIcon stockid="md-services-folder" resource="folder-services-16.png" size="Menu" /> | ||
|
|
||
| <!-- File icons --> | ||
| <StockIcon stockid="md-generic-file" resource="file-generic-16.png" size="Menu" /> |
There was a problem hiding this comment.
why the pattern (md-file-* -> md-*-file) change? Imo we should use the same pattern like we have for other categories (md-prefs-*, md-file-* etc.). Also the Stock IDs belong to the public API too, kind of. AddIns can reference and use them directly (instead of Gui.Stock). I remember that we changed some id's before and it was really hard for AddIn devs to track that, since we don't expose ALL icons throught Gui.Stock. @slluis?
There was a problem hiding this comment.
Right, I can use md-folder-* and md-file-*. The reason why I did it was that there's a huge chaos in IDs. Take a look: sometimes they're reversed, sometimes there's an -icon suffix. A mess.
There was a problem hiding this comment.
I might be wrong, but I really think we should clean up these IDs if we want to have more extensions in the marketplace. There should be a clear pattern, or it will look like they're glued together from different sources (which they in fact are). We might break some APIs today, but if we design a pattern now, it will pay off later in the longer run.
There was a problem hiding this comment.
I'm not against cleaning them up, I'm just for a unified pattern and for not breaking AddIns that use the IDs directly. And we should definitely remove the -icon suffix everywhere. If we don't care about the IDs to be changed without an API bump, then it's ok, otherwise we should sync it with the next API bump.
There was a problem hiding this comment.
If @slluis agrees to rename icon IDs, I'd make sure we have same pattern for all names. Plus I'd document it so it's super-clear.
| public static readonly IconId ServicesFolder = "md-services-folder"; | ||
|
|
||
| // Deprecated folder icons | ||
| public static readonly IconId ClosedFolder = "md-folder"; |
There was a problem hiding this comment.
Nit: To make this easier in the future, in case we do other renames, I'd say the pattern should be:
public static readonly IconId Folder = "md-folder";
public static readonly IconId ReferenceFolder = "md-reference-folder";
...
public static readonly IconId ClosedFolder = Folder;
public static readonly IconId OpenFolder = Folder;
public static readonly IconId ClosedReferenceFolder = ReferenceFolder;
We have a huge chaos in file and folder icons. I am trying to clean it up here.
As a part of https://github.com/xamarin/icons/issues/194 I cleaned up our special folder references. Results:
md-component-folder-*andmd-folder-assetsare unused. I'll have more icons to use soon, so we'll work on references afterward.Please don't merge, this is WIP.