Support SVG icon in action menu

[bjarnef]

Prerequisites

• I have added steps to test this contribution in the description below

Description

I noticed it wasn't possible to use a custom SVG icon in a custom action menu.
In umb-editor-menu I also fixed a typo in separator (I guess this part has never worked).

Example code:

public class ApplicationComposer : ComponentComposer<ApplicationComponent>, IUserComposer
{
    public override void Compose(Composition composition)
    {
        base.Compose(composition);
    }
}

public class ApplicationComponent : IComponent
{
    public void Initialize()
    {
        //register custom menu item in the media tree
        TreeControllerBase.MenuRendering += TreeControllerBase_MenuRendering;
    }

    public void Terminate()
    {
        TreeControllerBase.MenuRendering -= TreeControllerBase_MenuRendering;
    }

    private void TreeControllerBase_MenuRendering(TreeControllerBase sender, MenuRenderingEventArgs e)
    {
        if (sender.SectionAlias == "content" && sender.TreeAlias == "content")
        {
            var coffeeMenuItem = new MenuItem("coffee", "Coffee Time")
            {
                Icon = "coffee-cup",
                SeparatorBefore = true
            };

            e.Menu.Items.Insert(e.Menu.Items.Count - 1, coffeeMenuItem);
        }
    }
}

Icon only work with old font icons and not is using a custom SVG icon in App_Plugins folder. Another issue is that it expect the icons to contain icon- in name, which it not neccessary has with a custom font icon or SVG icon.

It would of course work if developers updated code from Icon = "coffee" to Icon = "icon-coffee", but that would be a breaking change. In this case I have a custom SVG icon coffee-cup where is it enough with Icon = "coffee", but won't in the view because of the icon- prefix unless I renamed it to icon-coffee-cup.svg.

To make this work I have a property UseLegacyIcon (true by default) to MenuItem to still work the old format and the new format.

var coffeeMenuItem = new MenuItem("coffee", "Coffee Time")
{
      Icon = "coffee-cup",
      SeparatorBefore = true,
      UseLegacyIcon = false
};

Maybe there's a better name for this like UseCustomIcon or something else, but we would need something to track if the current (old) format should be used or the new format. In pretty much all other part of Umbraco UI the icon property includes icon- in name, but not here.

Before

After

I have attached the folder with the custom coffee-cup.svg icon here, so extract this folder to App_Plugins:
CustomIcons.zip

[umbrabot]

Hi there @bjarnef, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

• It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
• The automated tests all pass (see "Checks" tab on this PR)
• The level of security for this contribution is the same or improved
• The level of performance for this contribution is the same or improved
• Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
• If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
• The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

[nul800sebastiaan]

I've looked at this a few times now and to be honest I'm not crazy about introducing new properties. I was thinking if we could check if the file with prefix icon- exists on disk but that is way too much overhead to do each time. Not sure if we can find a better way to do this though.

[bjarnef]

@nul800sebastiaan yeah, not ideal but I think it is the only way without a breaking change since it currently expect the icon- prefix. With custom font icons we can't check if a file exists on disk and although many custom icons in Umbraco is prefixes withicon- it is not a requirement. Furthermore with SVG icons they don't need to be prefixes with icon- at all. In my example is is named coffee-cup.svg

Ideally it should have required the full icon name, but it was not the case as it currently always expect icon- prefix in view, which then doesn't work with custom SVG or font icons not prefixed with icon-. In all other part of UI where the icon- is part of the name. But since developers already have implementations with this, I can't see other ways to solve this without a breaking change.

[bjarnef]

@nul800sebastiaan another approach could be introducing an IconPrefix property with default value icon-.. In case you have an icon name (SVG or font) not starting with icon- you could set the property value to null or an empty string.

Not sure it is better or easier though.

Currently the challence is that with the data passed in to the property, er don't know if the full icon name should be prefixed with icon- or not.

[bjarnef]

@nul800sebastiaan I found a few places in code, which have icon- prefix in view.
https://github.com/umbraco/Umbraco-CMS/search?l=HTML&q=icon-

Not a big deal with date picker, but context menu as fixed in this PR and <umb-property-actions> as in can be used in custom property editors or in packages, where you might want to use a custom font icon or SVG icon. Currently it will work if the icon name is prefixed with icon-, but otherwise not, e.g. with coffee-cup.svg.

For example @mattbrailsford might want to use <umb-property-actions> component in Vendr?

These are the only places I have found, which is inconsistent.

[nathanwoulfe]

I've looked at this a few times now and to be honest I'm not crazy about introducing new properties. I was thinking if we could check if the file with prefix icon- exists on disk but that is way too much overhead to do each time. Not sure if we can find a better way to do this though.

Not sure that's an issue - getting all icons from IconService.GetAllIcons will return a cached dictionary, so overhead will be minimal. Dictionary is keyed by icon name, so ContainsKey("icon-foo") would do the trick...

[bjarnef]

@nathanwoulfe the chosen icon is specified in the controller. Ideally the full icon name would be specified, but it was not the case. If you wanted to use the icon icon-document you just need to use Icon = "document" as it would prefix with icon- in the view. However this won't work if you named a custom SVG e.g. coffee-cup.svg without icon- in name or the same prefix in a font icon name.

This is different from pretty much all other part of UI where we deal with full icon name.

So e.g. with "coffee-cup" it need to know if it should use icon-coffee-cup as it would use today or if it should use coffee-cup (full icon name).

[nathanwoulfe]

@bjarnef gotcha. Sounds like a new property would be the way around this, like you suggested. Something to configure ignoring/removing the default icon- prefix

[bjarnef]

@nathanwoulfe yeah, I think that if what we can do for now to support custom SVG and font icons, which may not be prefixed with icon-.

We also have other parts in code regarding legacy format, UseLegacyEncoding: e.g. https://github.com/umbraco/Umbraco-CMS/blob/v8/contrib/src/Umbraco.Core/Security/MembershipProviderBase.cs#L100

Or try search on legacy 😅 I imagine in v10 in may be a major overhaul to cleanup legacy supported code - at least regarding frontend in backoffice 😋

[bjarnef]

@ronaldbarendse @madsrasmussen I have a few open PR regarding missing parts to support SVG icons, but unfortunely unlike other part of UI, these icons didn't included full icon name. So to support SVG and not break developers existing implementations of custom actions I have used useLegacyIcon which is true by default.
https://github.com/umbraco/Umbraco-CMS/pulls?q=is%3Aopen+is%3Apr+author%3Abjarnef+support+svg+

Do you have any better suggestions to handle this?

[bjarnef]

We could also keep this approach using useLegacyIcon in v8, but remove this in v9 and require the full icon name instead, which probably would require some updates in core tree controllers.

[nul800sebastiaan]

FYI: we're not changing anything in the frontend for v9, we want existing App_Plugins to keep working as much as possible.

[bjarnef]

That's fine :)

But to solve this we still need a way to identify whether the icon is a core icon, custom font icon or SVG icon. Checking if the physical file exists on dish won't work for font icons.
And from icon name we don't know if the developer actually want to use icon-coffee-cup, coffee-cup (SVG or font icon) or any custom prefix, e.g. gmd- (could both be used for font icon, but you could also just have named an SVG icon gmd-coffee-cup). Some may add a prefix (maybe company based) to custom icons to avoid collide with core icons or icons in third party packages.

To avoid breaking existing stuff, it need to include icon- prefix by default.

I considered adding a IconPrefix property instead with value icon or icon- by default (you could then set this property to null or empty string and specify the full icon name), but I am not sure this is easier to understand? And the UseLegacyIcon property clearly indicates that it is something that need to be removed/changed in a breaking/major version.

[bjarnef]

@nul800sebastiaan is this still relevant. It seems no more new features will be added to v8, so it would be great if we close this if it isn't accepted for v8. Then we would settle for SVG icons can't be used for tree actions and property actions in v8 and consider implement this in v9.
However I still need an answer of how we want to handle SVG icon vs font icon as described above without a breaking change and looking for a physical file isn't an option with font icons.

[umbrabot]

Hi there @bjarnef!

Thanks for the contribution here and apologies if it has been a while since you heard from us. We have been in the very fortunate position of having lots of work to do. With this in mind, we are writing to let you know that with the release of the Long Term Support (LTS) version, 8.18, we have now moved into the support phase of Umbraco 8. You can read all about that here but to surmise, we will be keeping Umbraco 8 safe and well by releasing patching for security or regression issues if they arise but no longer will we do that for bug fixes. The same is still true for features, although we stopped merging those some time ago.

We'd love for you to keep contributing and while we are not able to merge this to Umbraco 8, if this is still something you'd like to see in Umbraco 9, please take a look and either create an issue to say so or find an issue that already exists. We'll be happy to give you some input around how you can adjust your pull request to target Umbraco 9. Even better, it might be something that Umbraco 9 already does or has. In which case, enjoy!

Once again, a huge thank you for the time you have spent working with us.

#H5YR
Your friendly Umbraco GitHub bot 🤖 🙂