🇨🇦Canada @m4olivei

MR looks pretty good. The other core issue, ✨ Provide a Config Action to add/edit a component in a layout Active is moving along since last mentioned here. When I looked over it, it seems like it would cover our use case. It's also been marked as a Starshot blocker, so the timeline should fit our needs here.

@plopsec, what do you think? If it covers everything we need, this issue would no longer be relevant correct?

Added 🐛 Second level menu items can't be reached if they have children Active under "Could have"

Had a chance to catch up with @ckrina today. The design that @laurii referenced in #30 is indeed the latest iteration of the design to accommodate this issue. Let’s update the MR and/or close the current MR and open a new one for this approach.

Here is a representative screenshot:

Here is a link to the Figma: https://www.figma.com/design/VCPAxetieAC2pzw7hgX3ij/Drupal-Admin-UI---De...

A couple of caveats to keep in mind:

• "Overview" is a working title for this link. Seeking some UX input on that. Other considerations might be "See all".
• The Figma designs show a fourth level ("Great Grandchild Item"). This is the subject of 💬 Support Deeper Navigation Levels Active and is out of scope here.

I've also updated the issue description to match.

Follow up issue: https://www.drupal.org/project/drupal/issues/3488768 🐛 Menu blocks specific to Navigation are leaking into the Block layout UI Active

m4olivei → created an issue.

RTBC +1 for me!

Worth noting that, while testing this one, I found an existing bug where we don't properly remove all the navigation_menu block derivatives, such that when you are using the Block layout UI, you will see the Navigation specific menu blocks, eg.

To be clear this is not a blocker for this issue, as it was already present in 11.x. I'll file a follow up for that.

Let’s leave this one open for the time being until we can get @ckrina's input per @lauriii's comments. As you pointed out @michaellander, this has popped up a few times, so it's a pain point for some. We should at least document the decisions made. 📌 Parent menu items with URLs are not linked to, do we make these available? Closed: outdated comes close to that, but it was from a time when the design was much less settled than it is today.

I know there have been more recent design discussions on this topic. I'm attempting to reach out to some folks that worked on the designs to see if they can lend some insight into more recent discussions. Otherwise, I agree we should get @ckrina's input here before closing it out.

Adding a couple of issues marked stable blocker that were not in the issue summary here under "Must Have".

Removing Navigation stable blocker label. We had decided it didn't reach that threshold a little while back. This is reflected in the meta-issue 🌱 [PLAN] New Toolbar Roadmap: Path to Beta & Stable Active .

Removing Navigation stable blocker label. We had decided it didn't reach that threshold a little while back. This is reflected in the meta-issue 🌱 [PLAN] New Toolbar Roadmap: Path to Beta & Stable Active .

Thanks @plopesc! That makes sense. Thanks for the updates.

I've written a draft change record.

Also, updating the description to remove one more reference to footer_top as well as updating the screenshot to reflect the actual implementation.

As we only bumped back to needs work on docs related issues, marking this all the way to RTBC!

Looking good to me!

The MR doesn't address footer_top? I'm wondering if there was a specific reason why not that I'm missing. We either need to add that in as well, or update the issue description to reflect only content_top being covered here. Marking as Needs Work for this reason, but not wanting to discourage integrations from leaving thoughts as well, so please do feel free to leave review comments.

Can we use a third party settings ?
I think it would be easier to use as developer AND as site builder

Aren't third_party_settings used for with config entities? In this case we're looking to limit on the basis of block plugin id, not on a specific instance of a block config entity, or other. This is a developer-focused way to signal that their integration has been fully considered for use within the Navigation, given its constraints.

Setting it back to Needs Review for now, given open question from @matthieuscarset. Came in while editing my message.

Couple of observations. First, I noticed that 📌 Support navigation module Active is marked as a Starshot blocker, so you could argue that it should be available for us to use instead of doing this. Although, my read on that issue is that the scope of it is big and it'll be awhile. Not to mention it hasn't had recent activity at all. In that sense, it would be great to see this issue land to unblock other integrations cleanly. The eventual deprecation of this hook in favor of whatever the new approach will be is kind of annoying but should be trivial to do both approaches for a time, so not a huge deal. I'm also not super clear on if the proposal in 📌 Support navigation module Active would cover our use case. The block restrictions piece sounds very site builder focused, backed by config entities. I'm not seeing how you would be able to op-in a set of blocks in code. I posted over there to ask.

All that being said, the MR looks good to me. I've tested it locally and it works just fine with the existing set of blocks (no regressions). List of blocks remains limited to the default set we want to allow.

As @trackleft2 mentioned, the navigation module would also benefit from this work. That's being tracked in 📌 Provide a way for other modules to flag block plugin implementations as 'navigation safe' Active .

Although I wonder if the proposed resolution here would cover our use case. The use case for the navigation module is one where we want developers working on new blocks to opt-in their block to be used for Navigation. We currently do this in navigation module by implementing hook_plugin_filter_TYPE__CONSUMER_alter, and filtering out all blocks except a hardcoded list of exceptions. 📌 Provide a way for other modules to flag block plugin implementations as 'navigation safe' Active is currently looking at using a hook to be able to add / alter that hardcoded list. Would the proposed resolution here solve for our problem as well?

Suggesting a different name of 'header' to go with existing 'content' and 'footer'. Let me know what you think.

Also, do we want to address the region just above the footer, which is asked for in the issue description?

Otherwise, I think this is a fine way to cover this requirement. It offers a lot of flexibility to module authors to put whatever render array they want there. To @finnsky's point he's been arguing for in various places, we'll have to flesh out our components more in order for module authors to author things that fit well in these spots.

Tiny nitpick found to change t() to $this->t(). Otherwise looking great to me. +1 RTBC.

See world explode.

Can confirm. Tried on 10.4.x. Enabling navigation alone, all is well. Enabling tour, world expolode.

The open MR fixes the world from exploding (aka the WSOD).

and i guess this issue is not necessarily a duplicate of the other issue. cuz i've tested on 11.x with #3473594: Improve the tour block in the context of the navigation module and i run into a WSOD (probably the same WSOD @penyaskito ran into?)

I agree. It would be the same issue there. This is stemming from a hot off the press merge to core in 📌 Migrate Toolbar button to SDC Needs review (literally today). I would suggest merging this fix to mitigate the WSOD, and then continue with the visual work on the button in 🐛 Improve the tour block in the context of the navigation module Needs review . The visual result after this MR is the same in screenshots on 🐛 Improve the tour block in the context of the navigation module Needs review , so feels safe to merge and continue on there.

Re-opening and marking RTBC.

plopesc → credited m4olivei → .

m4olivei → made their first commit to this issue’s fork.

@ckrina there is precedent for this in the toolbar module. It has print styles to hide the toolbar for @media print:

https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/toolb...

Do you think that is reason enough to do the same thing for Navigation module as well? To be sure, it doesn't look like there was a very robust discussion for/against this in the issue where it was added. See #2403639: Remove toolbar from print view → .

Adding Navigation stable blocker tag, as this is blocking 📌 Agree on a lazy load strategy Active .

Postponing on 📌 Provide a NavigationLinkBlock Plugin and use Help as an usage example Needs review (which is really close!!), per last comment by @plopsec.

I've closed 🐛 Page break error display :The "logo" entity type does not exist Active as a duplicate of this issue, since we'll resolve that here.

Let's credit: pooja_sharma, kumudb, skaught, plopesc, and smustgrave for review / patches when we have a commit here please.

Let’s close this as a duplicate of 📌 Store the file path instead of ID for the navigation logo Needs work , and continue efforts there, and also remember to transfer credits. I'll make a note of those who contributed here on the other issue.

Linking to the core issue to do icons in a standardized way across Drupal core.

Also note, if we have agreement, @finnsky we can delete the change record.

I found a regression with the use of the new link_tag Twig function in Olivero. That obviously weakens the case for it, as navigation would be the single use of it. Added to the hesitation for adding another Twig function, I've removed it and instead taken inspriation from Olivero's template, using the same conditional for our purposes of deriving the HTML tag name from the URL object.

Should be all set now. Hopefully will be easier to get through without the introduction of the Twig function.

hestenet → credited m4olivei → .

Thanks for your work! It's great having your work on env indicator to get a look at the real world pain points in integration.

Adding a related issue around Navigation's extensibility. I'm not sure ATM if we want to merge the issues or keep both.

Good news!

@pdureau, @Grimreaper and others, as part of the UI Suite team, are working on a way to provide icons in a standardized way across Drupal. Per thread in Slack, they will be proposing:

• a YML plugin declaration of icons sets to be added to modules and icons with:
• • metadata (label, description...)
  • a way of discovering icons of the set based on configurable rules
  • options in JSON schema (a bit like SDC props)
  • an inline Twig template for renderable
• the discovery system is also bsed on Drupal plugins, and extendable by adding PHP classes
• integration (based on a shared form element) with :
• • Field API
  • Menu alters
  • Ckeditor5 API
  • ...
• Library pages

Lots of the same things that we are talking about here, and more. Therefore, we will postpone on that work, which should become available this week.

Part of the goal here is to allow for a contrib module to provide a library of icons that a site builder can assign to menu items in the UI. For that purpose, we would want to be able to provide some metadata along with each icon, to provide a clear, accessible UI. Hence the label, description, and weight keys. We also need a way to get a list of all available icons. Plugins would provide both.

The module already provides a CSS class based on the menu item plugin id. Granted, that simple approach could be slightly improved by setting the class on hook_menu_links_discovered_alter as you suggested. The way things are now, the class is cumbersome to override.

Ahh ok thanks @_nod for clarifying. I had not tested it installed on 10.3 and then upgrading to this branch. Makes sense then.

All ready for reviews! This will be a big one when it lands, b/c it opens the flood gates to getting the rest of the themeing from the navigation module switched to SDC.

Merged the latest from the 11.x branch as 🐛 Single directory component CSS asset library not picked up in admin theme immediately after module install without cache clear Active was merged recently 🙌. In my testing locally, it doesn't look like the empty update function is needed anymore, but I'm not sure why that would be as #3460598 just added clearing the discovery cache on install.

Been thinking about this one and studying the field_ui approach mentioned in #7 and #12 by @amateescu and @catch. I think that would do well for us. Here are the broad strokes of how I propose the field_ui solution would translate to navigation:

• We define a new YML plugin for navigation icons. A module could define one or more navigation icons in a MODULE_NAME.navigation_icons.yml file.
• Each plugin would have the following structure:

NAVIGATION_ICON_NAME:
   label: STRING
   description: STRING
   weight: INTEGER
   libraries:
     - STRING

• description, weight, and libraries are all optional. label, description, and weight are all future proofing here, maybe there would be a contrib module to list all navigation icons, and it might be useful to have those properties.
• Menu links would reference a navigation icon using a navigation_icon property. Similar to field_ui's category property on FieldType plugins.
• Set the navigation_icon property via hook_menu_links_discovered_alter for relevant menu items supported by Navigation module out of the box. Could also provide it via MODULE.menu.links.yml where applicable for core modules. Just like in @finnsky's POC. Make sure our hook runs last. This will cover all menu links included in MODULE.menu.links.yml files as well as those provided as menu link content entities.
• Similarily for contrib modules, they could ship their navigation_icon plugins along with their module. They then connect them to their own menu links, or alter the reference in via hook_menu_links_discovered_alter. Or allow a theoretical module that implements a UI to make the connection.
• On output/render, reference the navigation_icon property, include it in CSS class name attach any referenced libraries
• Libraries define CSS that will style against the expected class name

With that in core, it would allow for something like the proposal in #21 by @jidrone, which is a great idea, but probably beyond the scope here and more suited to contrib as has been mentioned by @ckrina. The contrib module could come along and fully flesh out all navigation icon plugins for an entire library like Phosper. It could further introduce a UI to configure an icon for menu link content as suggested.

How would you all feel about this approach for solving the scope of this issue? If we get consensus, we can move forward with an implementation issue.

I also want to suggest we take the opportunity to reconsider if this represents a Navigation stable blocker, while we're thinking about this, given ✨ If no icon for a top-level item is provided, use the first two letters Fixed has now landed since this was marked as such.

m4olivei → made their first commit to this issue’s fork.

I see now that a follow up was created for 📌 Store the file path instead of ID for the navigation logo Needs work .

Looks great!

Rationale in #7 is sound and vibes with the time that I spent in xdebug around the issue.

Following the steps to reproduce in the issue summary, I not get the actual result, eg. immediately after install, the expected CSS assets from the SDC in the sdc_cache_isse module are served to the client.

I've updated the issue summary to better describe the actual issue, letting #7 do the work, as it's so well written.

RTBC for me. Happy to write up a CR if necessary.

Added the empty update function to force a cache clear on update.

The theme logo UI lets you upload a managed file, but the configuration stores the file path, and it also lets you enter a file path directly. This means even if you rely on an uploaded file, it's possible to change the local configuration to point to the file path without having to know the specific file ID.

🤔 Great point. It makes good sense that the navigation logo would follow the theme logo UI more closely. Although the theme logo doesn't do any management of the size of the image, neither is enforcing a size on upload, adjusting the size on upload, or applying an image style on output. Are you suggesting that we follow that same approach wholesale, in which case this issue is not necessary? Or are you still OK with the approach here and wanting to add the consideration for better config export support in a followup issue?

This would then provide a way for site owners to use SVGs, which would partially solve #3436520: Decide if and how we should support SVGs on the Nav Icon then.

Agree it would help that issue.

Thanks @pdureau, great suggestion. I've added a commend and variable for clarity.

@sourojeetpaul I've taken a crack at re-stating the issue in the description. Hopefully that makes the issue clearer.

@finnsky Hopefully I'm still accurately describing the issue. I re-wrote most of it 😬.

This is looking good to me. All of my feedback has been addressed as well as the points from @pdureau in #29 from my POV. Items and notes for that are below just to give full details. Marking as RTBC for me!

render filter on string?
text is a string prop, so why executing render filter on it? text|render|trim

✅ This has been fixed.

To be split?
it has no type and no description in the definition
the two slicing operations is looking weird setAttribute('data-index-text', text|first|lower).setAttribute('data-icon-text', text|render|trim|slice(0, 2)|join('')) .
Are you sure it is not 2 different props?

✅ Leaving this as one prop is the right approach, IMO. What we're doing here is deriving a two-letter acronym for use when a menu item doesn't have an assigned icon. The requirement is to always use the first two letters. The API for the component is simpler to just ask for the one prop, rather than force the consumer to pass both and do the work before passing in. We can also enforce a two-letter acronym this way.

Bad markup when empty
Also, a condition or a default value is missing because when text prop is empty or missing, we get this markup:

<button class="toolbar-button" data-index-text="" data-icon-text="">

✅ This has been fixed.

Missing titles
Some props have a description but not title. It is better to add titles, they provide valuable documentation and can be used by UI leveraging the components.

✅ This has been fixed.

attributes prop
This prop is automatically added to every component, so why doing manual declaration here:

✅ This has been fixed.

Say there is a design change to increase the image size to 45*45 pixels, then all the existing uploaded images would be too small. Could we not use an image style instead? It would mean navigation module needs to ship a new image style, and then render the image via that, but should be less code overall too.

The first pass at the implementation here did use an image style. However, that approach was abandoned for reasons. See #35 - #39. Namely @plopsec had the following concerns with the image style approach, which weren't sufficiently addressed, and thus we shifted to the current approach.

• Image style could be modified by site administrators, leading to unexpected results
• Image Style could be deleted by site administrators, so we cannot assume that it is present and need to have a fallback in every place we load the custom logo image
• Would force to add anew dependency on drupal:image for Navigation

I'd argue that the design is pretty stable, and should there be a future adjustment to the image size, sites would still have the recourse of re-uploading their custom logo.

m4olivei → made their first commit to this issue’s fork.

What happens if the module is installed via drush instead of the UI?

It depends. The following produces the expected result:

$ ddev drush si -y minimal
$ ddev drush en -y sdc_cache_issue
$ ddev drush uli

However the following reproduces the bug noted here:

$ ddev drush si -y minimal
$ ddev drush uli
[use login link to view the site]
$ ddev drush en -y sdc_cache_issue

So it would appear that there is some core code managing the front-end theme cache when the module gets turned on, but not the admin theme, and so the admin theme cache as it concerns SDC assets is stale.

@finnsky I agree, there is pretty clearly some caching issue with SDC module components at play. I've filed the following issue: 🐛 Single directory component CSS asset library not picked up in admin theme immediately after module install without cache clear Active . It includes a sandbox module that I threw together as a minimal working example of the issue: https://git.drupalcode.org/sandbox/m4olivei-3460588

m4olivei → created an issue.

Recent changes look good.

I came across one weird thing with this MR that I can't replicate on the 11.x branch. Here are some steps to reproduce it:

• ddev drush si -y standard
• ddev drush uli
• Login and enable navigation module via UI, eg. go to /admin/modules, enable Navigation module and Save

Expected behavior

Navigation looks and feels like its supposed to.

Actual result

If you clear cache, things do start working as expected. Not sure what is going on. Cannot reproduce on the 11.x branch, so seems like something here introduced it.

I read the issue summary, comment and the MR. There is a remaining task of another issue but it isn't clear if that needs to be completed first. Does it?

I don't believe so. It doesn't need to be completed first. The added functionality here addresses logos that are too big. However, just like it is on 11.x at present, a user can still upload a logo smaller than the recommended dimensions, and we need a fix so we don't break the layout, which is the subject of ✨ Center small navigation logo on collapsed state Needs review . It's not a requirement for this issue. I've updated the issue summary. @SKAUGHT or @plopesc can correct me if I'm wrong.

Tests were also added and are already present for this feature as part of 🐛 Custom Navigation logo is disconnected from new Layout template Fixed .

I think we're save to close this one. We already have test coverage in place for this, and it will grow as part of 📌 Adjust custom navigation logo dimensions on upload Fixed .

Closing as outddated.

m4olivei → made their first commit to this issue’s fork.

This may not be needed b/c 📌 Adjust custom navigation logo dimensions on upload Fixed adds functional tests for this feature. It's close to being committed. Lets postpone on that one.

I'm unable to reproduce this issue with the latest 11.x.

Following the steps to reproduce in the issue description, I don't get the error reported in #8, the Navigation blocks UI does render. Furthermore, I can click on Save successfully without any issues.

Closing as cannot reproduce.

Just a tiny thing that I noticed affecting some of the screen reader text.

Otherwise, changes look great to me! This is a really nice improvement by my read. Curious to see what the subsystem maintainers / folks with more familiarity with SDC will come back with. Nice work @finnsky

Thanks @quietone. I've updated the change recode and issue summery per your suggestions.

Back to RTBC🤞.

I think we're all set on all threads. Tests now passing and ready for a re-review.

Hey there 👋. I was attempting to use menu_link_attributes to test something on Drupal 11.x and found this issue. I don't believe there is an actual problem with the code in question on Drupal 11.x or any version.

First, it was not obvious to me how to reproduce the issue or trigger the code path in question. Here's what I found to work:

• Checkout the latest Drupal core 11.x
• Install Drupal w/ Umami demo (eg. drush si -y demo_umami
• Clone the menu_link_attributes module
• Edit the menu_link_attributes.info.yml to add || 11 to core_version_requirement
• Enable content translation for menus. Navigate to Configuration > Region and language > Content language and translation. Enable "Custom menu link". Expand "Custom menu link", enable "Translatable" next to "Custom menu link". Enable the "Hide non translatable fields on translation forms". Click "Save configuration"
• Create a custom menu link. Set some menu attributes.
• Translate the custom menu link and click Save.

After all that, the last step, saving the translated custom menu link will trigger the code path pointed out. There are no errors or warnings here when I run the module in Ddrupal 11.x. This makes sense, b/c the code here, eg.

\Drupal::entityTypeManager()
      ->getStorage($menu_link->getEntityTypeId())

Will load an object of type \Drupal\menu_link_content\MenuLinkContentStorage, and that class implements in it's interface hierarchy \Drupal\Core\Entity\RevisionableStorageInterface, which has the legit loadRevision method.

I think the error you're getting is simply from static analysis. At runtime, there is no actual issue.

Marking as Needs Review. Suggesting to Close as works as designed, and then open a follow up to get to 11.x support.

m4olivei → made their first commit to this issue’s fork.

That was a really straightforward fix per the change record → in 📌 Adjust how Help link and Content links are removed from the Administration menu for the Navigation bar RTBC . Setting to RTBC.

Sad trombone. This is now failing Kernel tests with the following error message:

Drupal\KernelTests\Core\DependencyInjection\AutoconfigurationTest::testCoreServiceTags
Service 'navigation.event_subscriber' in core/modules/navigation/navigation.services.yml should not be tagged with 'event_subscriber'.
Failed asserting that 'event_subscriber' is not equal to 'event_subscriber'.

/var/www/html/core/tests/Drupal/KernelTests/Core/DependencyInjection/AutoconfigurationTest.php:33

Appears to be a new check added in 📌 Enable service autoconfiguration for core event subscribers Fixed . I'll work on the fix.

This is looking good. All remaining threads are resolved. I've just merged 11.x to the branch to freshen it up and see how the pipeline is. Will wait on results and mark RTBC if all is well.

Hey there 👋. I understand the role, and agree to follow the statements in the linked document. I'm happy to serve in this way. Thanks!

I took a crack at breaking the standstill on the one issue. Lets see what we all think.

Can confirm that all comments have been addressed. Everything is working well from a functional perspective.

• Installing the module from fresh sets the expected block config for the footer navigation region.
• No visual regressions on the Help link placement or otherwise.

This is looking good to me! Items in #43 have been addressed. Tests are passing and making good sense to me.

Addressed comments from @plopesc. Ready for review.

+1 RTBC. I tested locally and it works as you might expect:

I'm also unclear if I could have put it back to RTBC in this instance? Would be curious to learn process there too. Thanks!

Updated the issue summary as well as drafted a change record. First time drafting a change record, so let me know if I missed the mark in any way.

I'll take a crack at writing a change record here.

Updated the merge request to address the <h4> issue. See MR comment for details.

This is looking great. RTBC for me.

m4olivei → made their first commit to this issue’s fork.

Updated the issue description summarizing comments in favor of using the Megaphone.

Love the thought here. One thing I noticed is that the spacing in the footer has some regressions.

Will also go through a static review shortly.

I've merged 11.x and made the changes requested in #6. Tests are now passing.

Ready for review.

m4olivei → made their first commit to this issue’s fork.