Adds icons from App_Plugins/**/Icons/*.svg #8884
Conversation
There was a problem hiding this comment.
Super cool @skttl! It should be the other way around though, if the icon doesn't exist in App_Plugins, then use the one that ships with Umbraco. That way you can override any icon that we ship with. As described here: #5606 (comment)
|
Yup, updated now :) |
# Conflicts: # src/Umbraco.Web/Editors/IconController.cs
|
@skttl Good stuff! I tried to review this yesterday but I got a bit lost in what was happening. So I gave it another try and made the code a bit more clear, so it wasn't doing too many things in the same Can you give it another look and let me know what you think please? |
|
Note: the iconpicker panel (for now) doesn't use the SVG icons any more, so to test you will need to observe the icon after it is picked:
This now turns interesting because I choose In other words, before merging this, we need to make sure the icon picker uses the svg icons again. |
|
All good to me. I too noticed that the icon picker wasn't using svg icons earlier. :) Do you need a PR for reverting that? |
|
Super excited for this one! For reference, I've created a new PR to reintroduce the svgs to the icon picker, without having to change any legacy integration, as well as adding some lazyload functionality. #9063 |
|
Both Mike's and this PR are now in, 8.8.1 will be a fun release! 🎉 Thanks for the work @skttl! 👍👍👍 |
(cherry picked from commit 3b551bd)
|
Cherry picked for 8.8.1 in d7bf980 |
| var pluginIcons = appPlugins.Exists == false | ||
| ? new List<FileInfo>() | ||
| : appPlugins.GetDirectories() | ||
| // Find all directories in App_Plugins that are named "Icons" and get a list of SVGs from them |
There was a problem hiding this comment.
This could have been made MUCH faster if a real convention was created, like a specific Icon folder within an app plugin like App_Plugin/MyPlugin/BackOffice/Icons but now we have to deep scan every single nested folder in all app plugins which is very costly. I realize we are now having to add caching to this due to performance but this would be one of the reasons why. Being able to just look at the top level app_plugins folders means you'd only iterate as many top level foldres that exist which would be very quick.
This can also be made faster by not allocating DirectoryInfo objects for every folder that is looked at. Use Directory.EnumerateDirectories instead.
It would be ideal to have a real convention for this - at least documented now so that we can make this a real convention with better performance in netcore.
//cc @nul800sebastiaan + @kjac
There was a problem hiding this comment.
Yep, I realized that this was not the convention (thought it would be App_Plugin/MyPlugin/Icons but App_Plugin/MyPlugin/BackOffice/Icons is better! 👍 Let's go for that.
There was a problem hiding this comment.
@Shazwazza agreed. When I was trying to work out the performance issues with icons, I briefly changed this code to simply get all SVG files under app_plugins, and then do a reverse lookup for each file path to see if they were in an "Icons" folder. It was a slight improvement in comparison (since it didn't have to do multiple enumerations over the file system), but still not really good.
This however is already out in the wild. Making a convention at this point would be a breaking change. Question is if we should simply live with this in 8 and make the breaking change in netcore? Or rip the bandaid off now?
There was a problem hiding this comment.
I though it would only find icon-folders in the root plugin folder.
I say we should rip the bandaid and get the convention in. I noticed @nul800sebastiaan removed the 8.8.1 tag, so maybe it was postponed to 8.8.2 anyway?
There was a problem hiding this comment.
Yes, has been postponed to 8.8.2! No bandaid needs to be ripped, this code is unreleased at the moment, so all good to get it done peacefully! :-)
There was a problem hiding this comment.
I wonder if it should be possible to have a second level of subfolders inside the "Icons" folder or if it would cause performance issues?
For example if using these icons https://remixicon.com they are grouped in subfolders.
We can of course select the specific ones needed unless you want to create a general package to re-use in projects.
It could be nice e.g. to map a specific folder with a name/category (maybe via a package.manifest) so e.g. in icon picker it could should a dropdown to filter icons in this category - a bit similar the property editors, which belong to a group 😎
Just an idea for future improvements 😁🦄
There was a problem hiding this comment.
@bjarnef we've already learned from this PR that scanning for icons in folders is expensive and a serious performance concern, it's been rolled back and not released in the current releases yet.
We're talking about icon metadata here: #9122 - that should account for grouping in categories (and maybe even multilingual categories) as well.
This reverts commit d7bf980.
This reverts commit d7bf980.
This reverts commit d7bf980.
Prerequisites
Description
As mentioned in #5606 (comment) it would be good to have a way of adding icons without having to put them in /umbraco/assets/icons.
So I added the functionality to the IconController, so it can search through
~/App_Plugins/**/Icons/*.svgfor more icons to add.This way it will be a lot easier for packages to add its own icons.
To test
Add some icons in a folder called icons inside an App_Plugin folder.
You can download this one as an example. It contains icons called
icon-allen-keysandlego-blocks.Open something using icons, ie. the doc type editor, and verify that the icons are available:
Also verify after selecting the icon, saving the doctype, and reloading the browser window, that eg.
/umbraco/backoffice/UmbracoApi/Icon/GetIcon?iconName=icon-allen-keysworks and returns the desired icon.