Support SVG icon in action menu

[bjarnef]

Prerequisites

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

Description

This is a new PR of closed #10757 to v9.

Currently this introduce a breaking change to specify full icon name (like in most other part of UI). Alternatively we could use the approach in #10757 with a UseLegacyIcon property to make it work with current icons and full icon name.

At the moment it is assumed the icon (font icon or SVG icon) always is prefixed with icon- which is not always the case and not a requirement in other part of the UI).

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);
        }
    }
}

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 🤖 🙂

[nathanwoulfe]

Hi @bjarnef just to clarify - do these changes introduce a breaking change?

Probably not one for this PR, but I'd love to see all those icon string served from Constants.Icons. Would make these sorts of changes cleaner, eliminate the potential for typos, and be ever so slightly more performant...

[bjarnef]

Hi @nathanwoulfe

Yes, this PR instroduce a breaking change as previous you could just specify delete as icon and it would use icon icon-delete (it requires developers to update icons in custom tree controller menu actions)... setting icon-delete would not work, unless you had a font icon icon-icon-delete.

Previous it did only work for font icons.

If we should avoid a breaking change we could introduce a property like UseLegacyIcon as in the old PR.

Checking for psysical file with same name isn't enough, så font icons are combined in a one or multiple files.. and regarding SVG icons it would probably require to search directories again including folders in App_plugins and Umbraco folder.

Besides that custom font icons wouldn't have worked either unless you use the icon- prefix, which isn't a requirement to use font icons.

[nathanwoulfe]

Ah, ok. Personally, I'd avoid adding the UseLegacyIcon and wait for v10 with a breaking change. Adding support for legacy just feels a bit dirty IMO - keep moving forward, not looking too far backwards 😄

[bjarnef]

Yea, that was what I was hoping for in v9 release, but unfortunely that didn't happen 😅

Hopefully in v10 then. It's a similar issue solved in #12062

@nul800sebastiaan @bergmania could we make these breaking changes in v10 and make these few missing parts consistent and possible to use SVG icons like in rest of the UI 👍

[bjarnef]

@nathanwoulfe anything I need to adjust regarding this? It would be great if we can resolve the last bits of the UI to support custom icons as well.

[bjarnef]

@nul800sebastiaan I have also updated this, so core menu actions no longer use the legacy icon, but specify full icon name.

[bjarnef]

Example for v9 with/without icon- prefix and custom SVG icon:

public class ApplicationComposer : IComposer
{
    public void Compose(IUmbracoBuilder builder)
    {
        builder.AddNotificationHandler<MenuRenderingNotification, TreeNotificationHandler>();
    }
}

public class TreeNotificationHandler : INotificationHandler<MenuRenderingNotification>
{
    private readonly IBackOfficeSecurityAccessor _backOfficeSecurityAccessor;

    public TreeNotificationHandler(IBackOfficeSecurityAccessor backOfficeSecurityAccessor)
    {
        _backOfficeSecurityAccessor = backOfficeSecurityAccessor;
    }

    public void Handle(MenuRenderingNotification notification)
    {
        // for all content tree nodes
        if (notification.TreeAlias == Umbraco.Cms.Core.Constants.Applications.Content &&
            _backOfficeSecurityAccessor.BackOfficeSecurity.CurrentUser.Groups.Any(x =>
            x.Alias == Umbraco.Cms.Core.Constants.Security.AdminGroupAlias))
        {
            var wineMenuItem = new MenuItem("wine", "Wine Time")
            {
                Icon = "wine-glass",
                SeparatorBefore = false,
            };

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

            var beerMenuItem = new MenuItem("beer", "Beer Time")
            {
                Icon = "icon-beer-glass",
                UseLegacyIcon = false,
            };
            
            notification.Menu.Items.Insert(5, wineMenuItem);
            notification.Menu.Items.Insert(6, beerMenuItem);
            notification.Menu.Items.Insert(7, coffeeMenuItem);
        }
    }
}

[nathanwoulfe]

Updated base to 10/contrib, will let tests run, review and merge. Can then be cherry-picked back to 9 if appropriate (I'd expect not, I'd like to think the book is closed for 9, outside of bugs/regressions)

[nathanwoulfe]

I'll even deal with the conflicts since I introduced them by changing the base branch 😄

[nathanwoulfe]

@bjarnef I think I need to hold off pushing anything - given the base is now v10, if I push changes I think it will cause issues in your repo as the original branch was v9-based... Not sure the best way to progress this, other than you retargeting the PR from a fresh v10-based branch?

[bjarnef]

@nathanwoulfe I think we can resolve the merge conflicts, so it is in sync with v10/contrib branch.

[bjarnef]

@nathanwoulfe I have now resolved this merge conflicts and should be in sync with latest v10 changes. The changes in the actions mainly updates the icon to use full icon name and sometime later in a later major version we can remove the useLegacyIcon.

[nathanwoulfe]

Hey @bjarnef this all looks fine, works nicely with v10 (updated the test case to use MenuRenderingNotification to add the coffee cup node, icon display correctly).

I'm a little confused by the test failures. Tests are failing because MenuItemList.CreateMenuItem<T>() contains a possible null reference return (the method explicitly returns null if no action exists), but the method is a nullable return type (MenuItem?) so I'm not sure why the test would fail on the null return, when the method allows it...

[nathanwoulfe]

And for reference, my test code:

    public class ApplicationComposer : IComposer
    {
        public void Compose(IUmbracoBuilder builder)
        {
            builder.AddNotificationHandler<MenuRenderingNotification, MenuRenderingNotificationHandler>();
        }
    }

    public class MenuRenderingNotificationHandler : INotificationHandler<MenuRenderingNotification>
    {
        public void Handle(MenuRenderingNotification notification)
        {
            if (notification.TreeAlias == "content")
            {
                var coffeeMenuItem = new MenuItem("coffee", "Coffee Time")
                {
                    Icon = "coffee-cup",
                    SeparatorBefore = true,
                    UseLegacyIcon = false, // default value is true
                };

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

[bjarnef]

@nathanwoulfe oh, it seems something gone wrong in the merge as it has reverted the changes in v10 where e.g. MenuItemList.CreateMenuItem<T>() returns a nullable MenuItem.

[nathanwoulfe]

Maybe a new feature branch from v10/contrib, then cherry pick your changes into that branch and push a fresh PR...

I think merging 10 into 9 causes hiccups, probably take as long to sort that out as it would to cut a new branch

[bjarnef]

@nathanwoulfe closing this and starting moving stuff to a new branch from latest changes in v10/contrib branch #12403