Everything looks good to me here.
Test adjustment is great, see my MR comment for more details.
I just re-tested the latest on MR commit on the latest 11.x. Functionally it continues to work as expected.
RTBC for me. Great work @plopesc!
Thanks @plopesc.
We might need to decide as part of 5.x how the integrations with Navigation and Toolbar will be handled, since both would be hook based.
A new option for the toolbar_integration could be added to include the navigation plugin, or we could go for the submodule based approach, one for each of the navigation/toolbar approaches. I don't have a strong opinion here, though.
I'm a fan of adding a new option for the toolbar_integration config. Already an established pattern.
While testing this, found an issue that might need to be considered in this MR.
Nice find! I've pushed a commit to address this. In practice it seems to work fine, although I'm a bit worried it's missing the cacheability metadata for the layout builder config. Again though, it doesn't seem to matter in testing. It would be a bit of a pain to surface that up to environment_indicator_page_top, but not imposiible.
Regarding the related issue in Drupal core that would offer a better integration (avoid the need to configure the block for the Navigation sidebar): ✨ Allow modules to hook into top of content/footer sections of new core navigation Active . Providing some more detail to @plopesc comment in #47.
As @plopesc noted, it's now available in Drupal core >= 11.1.2 ( 11.1.2 release notes → ). It was also backported to 10.5.x, and will be available on 10.5.0 (not yet released). 10.5.0 is set to be released on June 16, 2025 (per Drupal core release schedule → ).
We'll want the current block-based approach for our 10.3.x, 10.4.x and 11.0.x users.
For >= 10.5.0 or >= 11.1.2, users would be better served by using the newly introduced hooks in
✨
Allow modules to hook into top of content/footer sections of new core navigation
Active
. For this, we should have a new branch of the module with these version constraints, an implementation of hook_navigation_content_top, as well as an upgrade path to remove any environment indicator block from the navigation to avoid a double rendering. I can work on this in a new MR tomorrow.
In summary, I'm proposing we:
- After review, merge MR 53 to the module as is, tag a new release under 4.x
- Create a new 5.x branch
- Prepare an additional MR here for 5.x that has an implementation of
hook_navigation_content_topinstead of the block plugin as well as upgrade path - The 5.x MR should adjust
core_version_requirementto be^10.5 || ^11.1.2 - After review, merge the additional MR to 5.x and tag a new 5.0 release
Open to feedback!
Fixed the issue noted with big_pipe by following an approach inspired by olivero, wherein we set a style attribute on the navigation template using the attributes variable. In this way the necessary foreground and background CSS variables are carried with the navigation markup itself, rather than living apart from it, globally in the <head>.
Reviewing this issue as it stands today.
From testing the latest. I found an issue with the styles when you first add the Environment Indicator block to the Navigation sidebar.
Steps to Reproduce
- Install standard profile on latest Drupal 11.x
- Enable navigation and environment_indicator module
- Login as root user
- Add the Environment Indicator block at
/admin/config/user-interface/navigation-block - Visit any other page on the site
Expected result
Environment indicator should display and be styled as configured (background and foreground).
Actual result
Environment indicator shows up, but is not styled with the correct foreground and background. Only once you refresh the page, will the foreground and background appear correctly.
I think what's going on here has something to do with Big Pipe. With Big Pipe disabled, we don't have the same issue. My guess is the issue has to do with adding the CSS variables in a preprocess hook, as an html_head #attached property.
Thanks @trackleft2.
What version of Drupal are you using in that test? I'm not seeing that on latest 11.x dev branch or 10.4 🤔.
Added a functional javascript test.
Also, I realize the issue description doesn't accurately describe the latest developments on this issue. Here is a crack at revising it.
This is ready for a re-revew. Summary of changes:
- Merged latest 11.x
- Fixed the issues noted by @finnsky around keyboard search. We now update
data-index-textto match the dynamic usename - Changed the default text of "User" to "My Account" to better fit other places this kind of thing is needed. Updated test to match
- Updated
\Drupal\Tests\navigation\FunctionalJavascript\PerformanceTest. We're doing less isValid checks? I think this is a good thing.
I also considered @catch's comment from earlier. For the reasons he mentions, but also as a way to address the small flicker @plopesc noted.
the js looks fine to me (which doesn't mean much because I am bad at js), although I wonder if there's a way to do it using core's existing lazy builder/big pipe workflow without additional js, but... I've never tried to do that with a menu link. And also we could look at that in a follow-up since it wouldn't be a data model or API change at all, whereas the new menu itself feels very stable blocking.
The trouble is that, as far as I can tell, the title of a menu link needs to be a plain string. I tried to create a new Menu plugin (\Drupal\navigation\Plugin\Menu\MyAccountMenuLink) for use as the top level default menu link. My idea was to override \Drupal\Core\Menu\MenuLinkInterface::getTitle and return a render array like:
public function getTitle() { return [ '#lazy_builder' => [ 'user.toolbar_link_builder:renderDisplayName', [], ], '#create_placeholder' => TRUE, '#lazy_builder_preview' => [ // Add a line of whitespace to the placeholder to ensure the icon is // positioned in the same place it will be when the lazy loaded content // appears. '#markup' => ' ', ], ]; }
This borrows from this approach in user module. The nice thing about this if it worked, would be that BigPipe would take care of intelligently replacing the username without our own custom Javascript, and handling the no JS case well too.
However, it white screens the site. Per the docs on the interface, that method should return a string.
Ultimately I don' t think the flicker is that bad, so at the most, I'd suggest we handle that in a follow up.
All done tweaks as per the design. Back to review.
Also, I'm hoping we can consolidate the approach to MR 53?
There were a couple of arguments against the sub-module approach earlier. When the hook is available as part of ✨ Allow modules to hook into top of content/footer sections of new core navigation Active that will further diminish that need IMO. @trackleft2 any thoughts?
Here are the design mockups in Figma: https://www.figma.com/design/ok4dy3tIkjCiWumnGSVWJP/In-Progress-(Public-for-Matt)?node-id=1-943&t=Yspt10m9NDQQqYzl-1
We decided to hide the two-letter prefix in place of the icon when expanded. We'll save that for later reconsideration if we want a feature request on this module to pick an icon (which would fit very nicely together with the rest of the navigation sidebar).
I'll push an update for the tweaks needed to the MR.
I'll also update the description here.
Hey there 👋. Just curious, does this update to the module mean that the following docs on the project page no longer apply? That is to say, on a quick read of the 2.x code it looks like cacheability metadata is now properly accounted for when using field_raw and field_target_entity
Cache warning
The field_raw and field_target_entity filters do not provide any cache information. Without additional measures content printed with these filters will not be update when changed in the backend. You can workaround this by rendering the field in your template but not display the result. For example: {% set catch_cache = content.field_image|render %}
I've updated https://git.drupalcode.org/project/environment_indicator/-/merge_request... per my last comment. Summary of changes:
- Fix a PHP error for an invalid component prop type
- Use modifiers prop for the
navigation:toolbar-buttoncomponent where applicable - Avoid style overrides to align with navigation styling. This gets rid of the gradient style on collapsed and uses a two letter acronymn as a persistent icon like menu items that don't have an icon.
- Remove unused argument to lazy builder, and code style
- Code style updates address all of @trackleft2's comments on the MR. Those can be marked resolved. I've marked those with a ✅ in the MR
Will mark as Needs Review for comments on that progress. Note that we'll still want to adjust for when ✨ Allow modules to hook into top of content/footer sections of new core navigation Active lands.
m4olivei → changed the visibility of the branch 3498074-user-name-on to hidden.
I ended up not liking the empty div. Instead, what I did was introduce a new #theme hook for content top to carry the cacheability metadata or empty items if present. This way we collect and render all necessary cacheability metadata, respect the API we set, and don't end up with empty divs.
I think this checks all the boxes, but I'm very open to suggestions.
Setting to NR in any case. Thanks!
Agree with @plopesc!
Also I spent some time yesterday chatting with designers who worked on the Workspaces mocks for navigation and had considered environment indicator along with it to be consistent. Here is some feedback, and I'm expecting some dedicated mocks for Environment Indicator that I'll post soon.
- The vertical line along the left of the navigation, colored to the environment was removed from more recent iterations
- The gradient used to trim the environment name when the navigation sidebar is collapsed is not a pattern that is used in navigation. We should follow the treatment for menu items without an icon and use the first two letters of the environment name as the icon. This has the benefit of drasticaly simplifying code here to override what core does. A further future improvement would be to allow site admins to pick an icon via the UI. That would depend on a couple of core issues yet, ✨ Add an icon management API Active and 📌 Decide strategy to customize or provide 1st level menu items' icons Active .
I've been working on those updates against the https://git.drupalcode.org/project/environment_indicator/-/merge_request... MR, and also incorporating @trackleft2's feedback there. I'll push those soon.
@trackleft2 correct!
However, the sub-module version of that issue hasn't received broad consensus per a read through of the issue. Also, both MRs require that the admin configure an additional Navigation Block via the UI. This patch will mean that's not necessary, it can be included via a hook and also be included in a consistent spot for all sites.
Will play a bit and work out a solution to my comment.
Added test coverage for this. Expecting the MR to fail.
m4olivei → created an issue.
Yep fair points. I'm only becoming more involved in core development recently. I'm still learning. There definitely is a human element at play that can be the difference between how fast issues move. This issue will also get more eyes being tied to Navigation, which is tied to Drupal CMS, which has a lot of resources focused on it ATM. Unfortunately, these types of things are not always well articulated in documentation and otherwise.
In any case, thank you for your contribution and thoughtful comments. Hopefully we see this one through to completion soon!
Test failures we're fixed by merging the latest 11.x.
Bug fix works as advertised and the test is super comprehensive. Nice work on that @plopesc!
Marking as RTBC.
@oily I want to encourage you to review the documentation on the issue Status → . It might clear up some confusion.
For this issue, and in general, you'd ideally have someone who did not contribute to the code to do a technical review and testing. That person, given they found no issues, could then move the issue to RTBC. RTBC issues remain in that state until a core committor has time for a final review before commiting. Once committed, it would move to Fixed. Then if there are no further comments or activity, it woul automatically move to Closed (Fixed). Again, review the issue Status → docs for more details.
I'll go through the MR to review and test. It looks like there is a test failure to handle. I'll see if I can work out why that is.
+1 for doing it in gin and/or at the Drupal CMS level somehow.
Doing it in core would introduce an additional pseudo status of experimental-but-really-its-fine → , which we'd probably want to avoid for other things that might be tempted to take that course as well.
Thanks for the review. I've addressed the feedback. Back to Needs review.
I have an hour right now, going to see what I can do to address open threads.
lostcarpark → credited m4olivei → .
Removed 🐛 Layout Builder Create Inline blocks logic should be optional Active per conversation in Slack from Must have, dropped to Should have.
On further conversation in Slack, we'll drop the Navigation stable blocker label, but keep it as a really important nice to have.
Addressed comments. Ready for review again.
Added 🐛 Layout Builder Create Inline blocks logic should be optional Active as Must have.
Thanks for reporting this @plopesc and doing all the work to get to a resolution.
The need makes sense. We don't want users to be able to add inline blocks to the navigation, it's not designed for that. Blocks for navigation should be very intentional and opt into placement there. I like that we're restricting inline blocks. We were discussing on Drupal Slack whether this is a Navigaton stable blocker. I'm of the opinion that it is, for the reasons just mentioned. I'm marking it as such.
I've tested the resolution and reviewed the code. It all makes sense to me and the tests look good too. I've taken it for a spin on my local, and all looks good. Marking RTBC.
This looks good!
I tested by installing the standard profile, and enabling navigation_top_bar module. I then created a new role with bypass node access permission, created a user of that role and logged in. As expected the local tasks were inline in the page.
I then gave that role access navigation permission, refreshed the page for the test user session, and local tasks were moved to the top bar as expected.
I've never written a performance test before, so please, any feedback is welcome. I assumed what was meant by "performance test" was \Drupal\FunctionalJavascriptTests\PerformanceTestBase (see
change record →
).
The tests I've added will first get the home page to warm caches. Then delete caches and get the home page again, this time collecting performance data. We then install navigation_top_bar and repeat the process, comparing performance data before and after, expecting that we get the same numbers, eg. navigation_top_bar does not impact performance. If top bar was mistakenly included for anonymous users, as was the case before the parent bug ticket was fixed, we'd see additional cache get/set after navigation_top_bar was enabled.
At least that is what I think I did 😅. My work was inspired by \Drupal\Tests\standard\FunctionalJavascript\StandardPerformanceTest. There wasn't any precedent that I could find in core for comparing before/after performance data like I'm doing here though.
m4olivei → made their first commit to this issue’s fork.
Updating the issue summary with final designs for a fourth level. Note that we have "Overview" links here. This is meant to link to the parent page, since the click on that parent expands/collapses the child menu, and does not click through. The "Overview" link funcionality is the subject of 🐛 Second level menu items can't be reached if they have children Active .
Removing Postponed status as 📌 Define the 3 areas the Top Bar will provide Active is merged to 11.x.
Removing the postponed title prefix as 📌 Define the 3 areas the Top Bar will provide Active is merged to 11.x.
CR written: https://www.drupal.org/node/3489732 →
Removing the relevant tags.
Thanks for the suggestions @catch.
I've updated the issue summary per #26.
Will work on a CR as well.
This is looking great to me.
I've tested locally and can confirm that the Help and Content links remain in the Administraton menu with the new approach, and there are no regressions to the Navigation menu.
Thanks for this one @plopesc!
Fixed merge conflicts after 📌 [ignore] Convert everything everywhere all at once Active was merged.
This is looking great. Just noticed two really tiny things in the menu wording. I don't see any other issues left to deal with.
The JS looks OK to me.
Noticing that we're still set to Needs work here. Is there anything else left to do here @plopsec that I'm not seeing?
All threads are addressed. Looking good to me. Setting to Needs review.
Test is green. Ready for review.
I've addressed the issue that was preventing local tasks from showing up where appropriate. I've also addressed adding some markup to get local tasks over to the right-hand side. There were already some grid styles in place for that. That can probably be further improved in follow-ups.
I'm looking at the failing test ATM.
I'm not really sure how to apply styles to nothing :)
Could you please add some test cases?
I also see that the local tasks are not rendering with the latest changes. Looks like we need to update the top_bar theme hook to be a render element type and probably do some preprocess.
I'll can work on that, as well as adding some basic styles.
@m4olivei The way LB is implemented in Navigation, nested in a different config entity makes impossible to use the trait approach described in #3475115: Provide a Config Action to add a component in a layout.
Ahh, now I understand. Thanks for explaining that. Our LB configuration is in simple config, whereas the config action in ✨ Provide a Config Action to add/edit a component in a layout Active needs to work on config entities.
I'll work on the remaining feedback here.
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 📌 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 📌 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.ymlfile. - 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_iconproperty. Similar to field_ui'scategoryproperty onFieldTypeplugins. - Set the navigation_icon property via
hook_menu_links_discovered_alterfor relevant menu items supported by Navigation module out of the box. Could also provide it viaMODULE.menu.links.ymlwhere applicable for core modules. Just like in @finnsky's POC. Make sure our hook runs last. This will cover all menu links included inMODULE.menu.links.ymlfiles 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_iconproperty, 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.