Navigation leverage icon core API

This MR is currently made of 2 parts:

• Everything inside /core/asset , /core/lib and /core/tests is a duplicate of 📌 Navigation leverage icon core API Needs work which was taken as a starting point. OF course, this part will be removed when the other issue is merged into 11.x
• the changes in /core/modules/navigation is the interesting part for this issue

Thank you for great start. I see some bem mistakes. Gonna fix it.

I don't think we should define viewbox somewhere

I don't think we should define viewbox somewhere

It depends of the icon pack we are implementing. Th goal of this Icon API is to provide simple tools to faithfully implement the icon packs as documented upstream by the icons provider.

For example, Bootstrap Icons expects a viewBox, so we need to deal with it:

<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" fill="currentColor" class="bi bi-emoji-heart-eyes" viewBox="0 0 16 16">
  <path d="M8 15A7 7 0 1 1 8 1a7 7 0 0 1 0 14zm0 1A8 8 0 1 0 8 0a8 8 0 0 0 0 16z"/>
  <path d="M11.315 10.014a.5.5 0 0 1 .548.736A4.498 4.498 0 0 1 7.965 13a4.498 4.498"/>
</svg>

In navigation module, we are both the icons provider and the icon pack definition author. So, we can do anything. If it works OK without the viewBox attribute, and you think it will be better without it, we can remove it.

I think we should keep the original viewBox attribute if it is not reassigned by the template

Here I added a simple illustration of how the absence of the original attribute breaks the adaptability of icons.

https://codepen.io/finnsky/pen/jOgKdWz

I will add the viewbox available as a variable if set in the svg, this will match your needs and be an optional feature when people need it.

I will update here when pushed on the main MR.

@mogtofu33 thank you for this and for all that initiative

@mogtofu33 hello!
It is possible to update that MR? Would be cool to have it onboard before stable release

mogtofu33 → changed the visibility of the branch 3483209-navigation-icon-api to hidden.

mogtofu33 → closed merge request !9938

Hi @finnsky, re-rolled!

The sidebar icons looks good, I have to check for other icons around navigation code, if you can have a look, thanks.

Thank you very much!
This is great step!

We also need to clean up some css tricks which we did for iconless buttons. I'll care about it in next days.

Hello @finnsky,

I updated the announcement and title buttons. But for the Top Bar icons, there is a vertical alignment to fix.

Probably not possible to replace the throbber icon override in toolbar-button. Not until the core throbber is an icon.

I am struggling to replace the toolbar-message icons (radioactive, help, warning) as I don't really see how to activate the messages, if you can point me I could probably do that.

Messages appear in Umami demo profile only for now.
https://www.drupal.org/project/drupal/issues/3441100#comment-15984062 📌 Integrate Umami message Active

Thanks @finnsky, added the toolbar message icon as well.

But because the naming need to match the icon, had to rename radioactive to error and copy help as status.
I let you decide how to manage that.

I didn't include a default icon for message in the twig. As I am not sure of the status for this feature, I just started simple.

Let me know if you need something else.

I looked at the MP! It looks great!

Here's what I added;

Since the icons have the same namespace, they can't be in different folders to avoid name collisions (spoiler: there was one already. Chevron in the button and menu item)
I moved all the icons to one folder

We can't use the toolbar-button--icon class for all the icons. Firstly, it doesn't follow the Bem style, and secondly, we need different classes. I added my own classes to the icons right in the config! Great!!!!

I removed the fill attribute from the icon. It's much better to set this from the CSS, especially since we have classes

Added icons to places where they weren't there yet. Different chevrons. Back button.

Corrected the styles and removed some hotfixes from recent tasks

Corrected the ajax animation

Corrected the messages

What needs to be done?

Need to fix tests

And most importantly:

If there is no icon, we should display the first two letters of the link name
https://gyazo.com/ed31b0f04e1d5ef87e278152bf1a98f1

In toolbar-button.twig
{% if icon is empty %} does not work
{% if icon|render is empty %} works

To check, add any link to the top level of the menu

Looking great!

The Icon API ignore folders as a choice to allow moving icons inside and organizing as will without impact.

Just added a small fix for the announcement icon upside down.

And as well I forgot the Icon twig function do not do anything to allow 'last' rendering from the icon RenderElement. So as you found you need to render.

I have a suggestion to make navigation menu compatible with ui_icons_menu → , which give the opportunity to add an icon to the menu, and would probably be compatible with any other module giving the opportunity to add an icon to the menu.

In toolbar-button.twig, we could check if the text is not only text but something more, like for ui_icons_menu the text include the icon as a FormattableMarkup. In this case, checking if text is a mapping would allow to ignore the non icon (2 letters) and let a custom icon be picked. The icon choice will even be part of the Navigation Icon pack! So you can even provide more icons not yet used in the current entries!

So would be line 27:
{% if icon and text is not mapping %}

You can quickly test by installing ui_icons, enable ui_icons_menu, create a menu entry and pick an icon.

Here is the result example, there is just a minor padding to fix that could be added in the ui_icons_menu:

Hi guys,

Things are taking shape, that's great.

In toolbar-button.twig
{% if icon is empty %} does not work
{% if icon|render is empty %} works

Early rendering must be avoided in a template, so no |render() filter here (maybe we need to deprecate this filter one day).

Let's not merge until {% if icon %} is enough. Is there something we need to change in the Icon API side?

yeah, seems so we need to add checking if icon in existing icons list. this should be condition.

Is there something we need to change in the Icon API side?

Seems no. Looking good! Thank you!

I have idea :)

I used the power of CSS

We couldn't do this before because we didn't have an element in DOM. But now we know that if there is no icon we will add an attribute. I think it's great

All that's left is to fix the tests. Yaay

Only one test failure:

2x smaller StylesheetBytes!!!

All tests green!

Checked the MR and progress here is amazing. I would really love to have it merged soon.

Just mentioning a few things to take into account:

• Tested locally, and it seems that this MR also solves 🐛 The icon for the more actions button is not visible Active . That issue might need to be updated
• Looks like some of the changes requested in 🐛 Undo some changes Active are being applied here. The scope of this issue and that one might be updated
• 📌 Provide a NavigationLinkBlock Plugin and use Help as an usage example Needs review Introduced a new navigation block that has to be integrated with the Icon API as well. I would open a follow up for it since the benefits of this one are more relevant than those adjustments and might need some unrelated architectural discussions

Code looks good to me and I can't appreciate any visual regression while testing manually, but I would love a second opinion from a more FE specialized reviewer before marking it as RTBC.

Created 📌 [PP-1] Integrate the new Navigation Icon API with NavigationLinkBlock Postponed as follow-up for the NavigationLinkBlock internals

@mogtofu33, @pdureau

I have question, maybe you have idea?

How technically modules which extend Navigation may set their own icons with current approach?

i'm thinking about
modulename.icons.yaml
and
{{ icon(‘modulename’, icon, { class: ‘toolbar-button__icon’, size: 20 }) }}

It can be SDC option:
icons_lib:
default: navigation

Modules like: (before they had CSS option for this)

https://www.drupal.org/project/navigation_extra_tools →

CSS option for extending still exists. But maybe we can make it ideal ;)

I'm reading the issue backscroll and the Icon API docs → , since I was not very familiar with it and led me to a couple of questions about how Icon API would integrate with Navigation in a generic way.

One of my main concerns from a long time ago is about how to map the icons with navigation items. Current approach is based on some magic CSS classes that could be implemented by Navigation module for the default core values or by any other module or theme to override those values or give support for new navigation items.

This icon based approach seems to be based in the match between the menu items and the icons provided by Navigation module. The Icon pack is hardcoded in the twig templates, so will not be easy for other modules to include their icons in Navigation. The CSS magic class approach is still there as a fallback, but would be great to figure out an Icon API based path for other modules and custom menus.

I could think of using hook_icon_pack_alter() to provide extra icon sources for the navigation pack, but not sure if that would work in a generic way.

Icon UI Menu module looks very promising but we cannot rely on that here unless that module is included in core, or at least part of the functionality, and I think we need to figure out an alternative with the current tools we have now.

I don't have a clear answer for this, but maybe Icon API maintainers could come with a proposal that would help to have all the benefits of the Icon API while giving some extra flexibility to the whole Navigation Icon ecosystem.

Well then let's leave it as is for now.

This MR already solves many problems on the frontend.
In the modules that use Navigation nothing will break and it will simply display the first two letters and the Icon can be added in CSS.
Which is quite flexible.

What is really needed here is good testing.

Really great work here!

I've done a first pass over the backscroll, static review and visual regression review. As well as familiarizing myself with the Icon API docs.

I've left a couple of comments on the MR. In addition, I found a couple of visual regressions:

1. The chevron has a smaller stroke width here than on 11.x.

2. The Top Bar edit button is regressed as well. It has the wrong icon now and the wrong font-weight.

I still have a lot of testing to do, including but not limited to:

• Windows high contrast (should be better here from static review - exciting)
• RTL testing
• Accessibility testing
• Review scenarios where toolbar-message.pcss.css gets used
• Review scenarios where navigation--message.html.twig gets used
• More visual regression testing

Thank you for starting review it.

2. The Top Bar edit button is regressed as well. It has the wrong icon now and the wrong font-weight.

They are actually structural fixes. Regressions were added previously.
Few of them out of scope of that ticket. But they should be done anyway.
https://www.drupal.org/project/drupal/issues/3505291 🐛 Undo some changes Active

They are actually structural fixes. Regressions were added previously.
Few of them out of scope of that ticket. But they should be done anyway.
https://www.drupal.org/project/drupal/issues/3505291 🐛 Undo some changes Active

I'll follow up with @ckrina on the Top Bar designs and their status. In the meantime, I do agree that the changes are out of scope for this issue. We should focus on the Icon API here, and not see any visual regressions from the state of 11.x. Just to ease the review for other folks coming in.

We should focus on the Icon API here, and not see any visual regressions from the state of 11.x

We should not compare the appearance with version 11. But with the design that does not exist.

In fact, only the color and font thickness of the button are beyond the scope of this task. But this button is not in the design. What is currently in version 11 is something random. In addition, it was added incorrectly.

So I don't see anything critical if it is here.

Further testing:

Windows high contrast (should be better here from static review - exciting)

PASSED

Looking great! Can confirm that we're seeing some fixes, namely with the icons, for the windows HC issues noted in 🐛 High contrast mode needs some more refinement Active . Will need to update that issue once this lands. Here are some screenshots of various testing scenarios:

RTL testing

PASSED

Looking good here! Here are some screenshots of various test scenarios:

Accessibility testing

PASSED

Tested with VoiceOver on Mac. Things are looking great here as far as I can tell. Can confirm that 🐛 Remove prefixed abbreviations from top-level menu items Active is resolved here, which is very nice.

Review scenarios where toolbar-message.pcss.css gets used
Review scenarios where navigation--message.html.twig gets used

PASSED

This is the Demo Umami message. To test need to install the demo_umami profile, enable navigation and look at an admin page. The message appears in the navigation correctly and fixes the bug in Chrome where the icon is not visible. Here are some screenshots showing all good:

Unrelated, but I did notice some slight alignment and spacing issues with the Demo Umami message. I'll file a follow up issue for this if we don't already have one.

Otherwise, I want to look through the code again, and consider @plopesc's comment.

This is looking great though.

We should not compare the appearance with version 11. But with the design that does not exist.

In fact, only the color and font thickness of the button are beyond the scope of this task. But this button is not in the design. What is currently in version 11 is something random. In addition, it was added incorrectly.

Checked with @ckrina. The designs for the top bar will still see further refinement, that will be followed by a design implementation issue at some point. It would still be ideal not to be seeing visual regressions from 11.x here, but I'm OK with it if others are, not willing to hold us up on that.

I've sketched out a POC for a mapping from menu item id to icon (icon_pack_id and icon_id). I've put that up as a separate branch and filed a MR against the 3483209-navigation-use-icon-api branch so as not to clutter that branch for now:

https://git.drupalcode.org/issue/drupal-3483209/-/merge_requests/1/diffs

The main bit is a new service, \Drupal\navigation\Menu\MenuLinkIconMap that has a static map from menu link id to an (icon_pack_id, icon_id) pair. There is also a new alter hook for this, hook_navigation_menu_link_icon_map_alter. That mapping gets used to avoid hardcoding the icon_pack_id and icon_id's in the templates. It also has the nice side effect of naming the icon SVG files in the way they were intended from the phosphor icon library.

This is meant as a quick way to setup a mapping that would allow others to alter it relatively easily. In future, it could be managed in config, with a UI, or glue it together with the existing ui_icons module.

Curious to hear what @mogtofu33 and/or @pdureau think about the POC, or any others.

Thank you for jumping into this @m4olivei!

Checked the MR and in general it adds architectural improvements to the initial approach, since icons are now more dynamic, could be overridden and allows other 3rd party modules to define new icons to be used. AFAIK we have navigation_extra_tools & dashboard, but could be more in the future.

On the other hand, the system is not very intuitive for non-experienced users. And I ma not sure how this would work with menu items created through the UI. We might need to implement a contrib module on top of ui_icons to handle it.

My other concern would be related to the fact that this new API might need to have a more or less defined plan from now to the date where all the pieces could come together and have a proper mapping between the menu items and the icons. That might require some help from a Framework Manager with a better understanding of the whole landscape.

Thank you again, this is a great step to open that conversation.

My other concern would be related to the fact that this new API might need to have a more or less defined plan from now to the date where all the pieces could come together and have a proper mapping between the menu items and the icons. That might require some help from a Framework Manager with a better understanding of the whole landscape.

The goal and the scope of this API was to be "the SDC of icons", providing lower level tools for developers:

• a discovery mechanism for icon packs
• an extractor plugin type to collect the icons from each pack
• a render element and a twig function to use those icons in code

Because we believed those pieces were enough:

• for Drupal Core, to solve the current issues with icon management (as we can see in the current issue)
• for Drupal Contrib, to consolidate the implementation of the numerous modules managing icon packs and icons, and make them more compatible with each others (this effort will be visible after Drupal 10 end of support I guess)

What we are not providing in this API:

• Form Element(s): but it may be a good idea to add one or two in a few months, picked among the best from the emerging ecosystem of contrib modules
• Higher level integration with Drupal config (menu, field storage, ckedtor5, text formats...) because we are expecting the emerging ecosystem of contrib modules to be more creative and dynamic with those tasks
• Other interesting but currently out-of-scope mechanisms, like allowing a icon from a icon pack to replace the icon from another icon pack (so a theme can replace Navigation icons for example)

We have an exciting future ahead

A note on the icon API override:

The core icon API do not provide (yet) a switch/override/weight system for the discovery, because it's based on the core YamlDiscovery there is an override based on provider name and type in this order: module by name, profiles by name, themes by name.
So if you create an icon pack named navigation in a module with name > n, this will override, if it's < n it will not.
If you have a profile with icon pack navigation it will override the modules, a theme icon pack navigation will override profiles and modules.
This is not intuitive but just how YamlDiscovery works to lookup files. I will try to add this to the current icon api documentation.

On @m4olivei POC

I like the more simple approach to not force any icon and rely on a mapping and some install config.

Your work on the NavigationMenuLinkTree suggest I should work on a core issue to allow MenuLinkTree to handle icon ids.

I agree with @plopesc on the not very intuitive, perhaps set the pack_id as a setting and the mapping too instead of hook would be better. You can have the pack list option from IconPackManager::listIconPackOptions().

Unfortunately there is not (yet) a core icon field, combined with a compatible MenuLinkTree it would make less work here. I have to try to work on that but I don't have much time currently...

I want to thank everyone again for their enthusiasm!

But also to clarify that this MR already makes many useful improvements!

Let's do everything to get it merged as soon as possible and continue discussing the icon configuration in a separate issue.

https://git.drupalcode.org/project/drupal/-/merge_requests/11172
For example, there are several comments here that we should address here, or do later?

This video explains a lot ;)

I'll check it

Thanks for all the feedback!

Let’s look at general comments, then look at POC comments:

General comments

From @pdureau in #45

This API is not currently providing: [...] Higher level integration with Drupal config (menu, field storage, ckedtor5, text formats...) because we are expecting the emerging ecosystem of contrib modules to be more creative and dynamic with those tasks

This is what we're running into here, of course. Navigation needs icons against each menu item displayed. The POC I put together is meant to address this in a light-touch way, but perhaps we can defer the POC approach or one like it to a follow-up issue.

From @mogtofu33 in #46

The core icon API do not provide (yet) a switch/override/weight system for the discovery, because it's based on the core YamlDiscovery there is an override based on provider name and type in this order: module by name, profiles by name, themes by name.

Neat! This is encouraging. At least there would be some way to override this prior to doing some sort of mapping like is done in the POC.

POC comments

From @plopesc in #44

On the other hand, the system is not very intuitive for non-experienced users. And I ma not sure how this would work with menu items created through the UI. We might need to implement a contrib module on top of ui_icons to handle it.

Agree it's not very intuitive; the POC as is definitely has developers as the target audience rather than a site admin. It would look like this:

/**
 * Implements hook_navigation_menu_link_icon_map_alter().
 */
function mymodule_navigation_menu_link_icon_map_alter(array &$map) {
  $map['menu_link_content:6db5002e-413e-482f-b971-a29b6c689f10'] = [
    'icon_pack_id' => 'navigation',
    'icon_id' => 'eye',
  ];
}

Getting that menu link id is non-trivial. It's kind of exposed in the markup as the id on the <li> element. Otherwise you'd have to either do some xdebug'ing, or look at the DB and know how to compose it together 💀.

From @plopesc in #44

My other concern would be related to the fact that this new API might need to have a more or less defined plan from now to the date where all the pieces could come together and have a proper mapping between the menu items and the icons. That might require some help from a Framework Manager with a better understanding of the whole landscape.

Yep, I hear this. This speaks follow up issue to me.

From @mogtofu33 in #46

Your work on the NavigationMenuLinkTree suggest I should work on a core issue to allow MenuLinkTree to handle icon ids.

+1 from me. I'd be very interested to see if core committers would go for something like that. Formalizing where that data gets stored would probably help ui_icons<code> module. Especially considering the challenge of storing the data for all menu items (both YML discovered, eg. <code>*.links.menu.yml and menu_link_content entities), see ✨ Menu: support non content menu links Active .

I agree with @plopesc on the not very intuitive, perhaps set the pack_id as a setting and the mapping too instead of hook would be better. You can have the pack list option from IconPackManager::listIconPackOptions().

Yep. More evidence that we probably need a follow up to fully flesh it out.

Next steps

I'm convinced the mapping should be a follow up issue to address.

To @finnsky's point in #47 there is some good stuff here. I'm going to try and address the two issues that I pointed out in the MR for this issue.

I rolled back the use of icons to the clean class.
Now everything works. And that problem from the video is no longer repeated.

Let's test!

To @finnsky's point in #47 there is some good stuff here. I'm going to try and address the two issues that I pointed out in the MR for this issue.

@finnsky's too fast for me! Will test the latest. Looks promising from a quick glance at the code changes.

Found two small issues in testing:

First, it looks like we need to rename the media.svg asset. If you install demo_umami, which has a Media link in the Content menu, you can see the icon is missing:

Second, in RTL the expand/collapse button chevron icon on the navigation does not rotate to respond to the state, eg.

https://www.drupal.org/files/issues/2025-03-03/rtl_expand_collapse_btn_d... →

Thank you for review. Gonna check now.

One other thing for us all to be aware of here, is that Gin will need to update its style overrides for the navigation icons after this lands. Thats not an issue, that I'm aware of, given the experimental status. Just FYI.

Gin on 11.x

Gin on this issues MR

Noticed that the issue description needed some love. Updated.

RTL issue and media icon issue that I noted in #52 are fixed. I've otherwise retested in RTL, Windows High Contrast, and VoiceOver. All are looking good to me!

I'm going to go ahead and say this probably could use a change record. Even though Navigation is experimental, it would probably be beneficial for community folks like Gin (see #55) to be aware of this change and have a simple note to follow to update.

small merge conflict. Other than that it looks good to me, great progress and thanks for the extensive testing! Feel free to RTBC, happy to commit this :)

Not sure about the change record. I'm guessing there are folks working on gin here no? we might add a note to the existing icon api change record but I'm not sure what would be in the change record? Also can't commit if we don't have the change record. so in the spirit of not making our life harder, I'll remove the tag.

Thank you all for participating!

nod_ → closed merge request !11172

Committed dd95187 and pushed to 11.x. Thanks!

Not sure about the change record. I'm guessing there are folks working on gin here no?

I don't think there is anyone here on this issue working on Gin. FWIW, now that this has landed, I've put up an MR for Gin that shows the changes required to have Gin continue to override navigation icons to customize: 📌 [PP-1] Update Core Navigation Icon Overrides Postponed .

we might add a note to the existing icon api change record but I'm not sure what would be in the change record?

I think what would be valuable is to document the YAML discovery override technique that I'm using in the Gin MR in 📌 [PP-1] Update Core Navigation Icon Overrides Postponed . @mogtofu33 suggested this in #46:

This is not intuitive but just how YamlDiscovery works to lookup files. I will try to add this to the current icon api documentation.

I'll take a crack at this.

I've updated the icon pack API change record: [#3490350]. Interested to hear from @mogtofu33 on that. Overriding an icon pack is possible, but it's not the best DX. That's not anything to do with this issue, but perhaps might inspire further improvements.

Automatically closed - issue fixed for 2 weeks with no activity.