plopesc → created an issue.
Created MR to allow this feature in core. ✨ Allow generic config entities to opt in to the entity specific Manage Permissions form Active .
MR created.
plopesc → created an issue.
MR created.
Tests are green, but PHPStan flags that we extend an internal class. Not sure if we could live with that. Tried other approaches, but this one was the less intrusive one for me.
MR created.
I did not realize until you mentioned it. Great catch!
Changes added to the MR.
MR created.
Included in 1.1.0
Merged!
MR added
plopesc → created an issue.
Nice addition! Let's get this in.
plopesc → changed the visibility of the branch 3478224-proof-of-concept to hidden.
New MR following a different approach where context responsibility relies on the plugins instead the Top Bar itself.
plopesc → changed the visibility of the branch 3491044-navigation-top-bar to hidden.
It looks good to me and improves the module's UX. Marking as RTBC.
Thank you!
I like the approach. This feature could be useful in the future.
Logic and tests updated. Ready for review.
Draft CR created.
Thank you for your review. Feedback addressed.
Checked and it looks good to me. Thank you!
penyaskito → credited plopesc → .
MR created including basic test coverage.
Thank you for your review @m4olivei
It was still in NW because @finnsky noticed some possible issues with the markup and JS. Also, when the navigation is expanded a small glitch is perceived when the username replaces the "admin" label. I would like to know if there is any possibility to improve that behavior.
All the comments by core committers were addressed and the basic styles implemented, I think this one can be marked as RTBC.
Made a quick review and left a comment.
Tests might need to be updated to ensure this new logic is working as expected.
Thank you!
@finnsky We only need the very basic styles to define the 3 regions:
- Tools: Aligned to the left
- Context: Centered
- Actions: Aligned to the right
Currently, only Actions has any content, the "More Actions" dropdown in entity pages.
Added a small comment regarding the default value for the exiting sites.
Would be necessary an upgrade path for existing sites to make this change transparent for them?
While we are here, would be great to have a simple test for this new feature.
Good suggestion!
Glad you find the module useful-
Name changed.
Thank you for your offering @dhruv.mittal.
Even if the initial approach was to use that "Default Dashboard" label for the original tab, we agree that the renaming is an improvement.
Feel free to work on it and let us know if you have any question.
@m4olivei The way LB is implemented in Navigation, nested in a different config entity makes impossible to use the trait approach described in ✨ Provide a Config Action to add/edit a component in a layout Active .
We discovered it when I tried to apply it to this one. See comments #6 & #7 in ✨ Provide a Config Action to add/edit a component in a layout Active .
If we want to provide config actions for Navigation, with the tools we have today, it should be based on a custom plugin.
Added tests for the current approach. They will be updated once ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active lands to support attributes.
Hi, magic happens now in hook_block_alter(), according to conversation in Slack (https://drupal.slack.com/archives/C7AB68LJV/p1731064940758699).
Modules can define their blocks adding the property allow in navigation, like in the MR:
$definition['allow_in_navigation'] = TRUE;
This will be moved to block attributes once ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active is merged.
Updated against 11.x and static analysis flags fixed. Ready for review.
Thank you for you review @m4olivei.
Footer is a more complex region than content and I would rather to discuss it in more detail in a follow-up issue.
Merging this one would unblock other integrations like Dashboard, Environment Indicator, Workspaces or Umami.
If you are OK with that, i think the only remaining step here would be to write the corresponding Change Record.
It seems a great idea!
Could you please mention the module in the core issue, so other people with similar needs could reach the module while the core discussion is happening?
I'm afraid that having this new widget in core might take some time.
Styles are applied properly and conditional logic for Gin theme is working as well.
Using Claro:
Using Gin:
Current approach worked for us, but we had some issues with internal tests, so we had to make a few minor adjustments.
Created MR with our last version.
Agree that it could cause some performance issues, but for now, it solves the bug while a new approach is defined.
plopesc → made their first commit to this issue’s fork.
@m4olivei Property renamed. Could you please take a look into this again?
Static tests are failing due to an unrelated issue in 11.x.
Created new MR !10103 following instructions provided by @larowlan in Slack to use Block Attribute properties to declare blocks as 'Navigation safe'. For now, using hook_block_alter. Native Attributes could be used once ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active is fixed.
This issue would make a difference and other modules like Navigation or Dashboard could take advantage of this new feature. Just one question about a possible scenario that I'm not sure whether current implementation would cover.
* Navigation adds a new 3rd party attribute for Block Attribute
* Module X provides multiple feature, one of them is a Navigation block, defining the corresponding Navigation specific block attribute allow_in_navigation
* For any reason, Navigation module is not enabled in the site
* Should the Error: Unknown named parameter $allow_in_navigation in ReflectionAttribute->newInstance()... be triggered?
How this scenario should be handled?
MR looks good to me. Thank you!
Agree that we need a streamlined way to approach this issue, however, it seems that this feature is actually needed by Navigation to help with the integration of other modules. It could be even a Starshot blocker, as described in 📌 Support navigation module Active
Created a MR that provides a hook based approach to declare 'navigation safe' blocks.
Once we have a more robust and global way to declare them in LB, as expected in ✨ Add the notion of a 'configured layout builder block' to solve a number of content-editor and performance pain points Active , the hook could be marked as deprecated and move towards the global solution.
plopesc → made their first commit to this issue’s fork.
Regarding the hook implementation, 📌 Provide a way for other modules to flag block plugin implementations as 'navigation safe' Active should be taken into account as well.
I don't think we can ship that config change as part of the Dashboard module.
Would be great if we could get ✨ Provide Config Action to add new blocks to navigation from recipes Active merged to allow to include the dashboard block as part of the corresponding Drupal CMS recipe. Another option would be to add the block to the navigation top programmatically, following the approach in ✨ Allow modules to hook into top of content/footer sections of new core navigation Active .
plopesc → changed the visibility of the branch 3318283-remove-count-in to hidden.
Good point about the enum!
Created and used it as part of TopBar::preRenderTopBar() . Tried to use the enum in the twig template as well, but it was not possible, given that enum support in Twig has been added recently and will not be available until 3.15. Core is now using Twig 3.14.
See https://twig.symfony.com/doc/3.x/functions/enum.html & https://github.com/twigphp/Twig/pull/4352
Regarding the styles, some basic styling is needed. Hopefully a more experienced FE dev could help here.
Patch looks good and works well for us.
Marking as RTBC
MR created including test coverage and CR draft created.
This is not an actual bug, but a feature request.
Working on it.
plopesc → created an issue. See original summary → .
Implemented an initial approach to create a new "promoted" region in the Navigation bar that allow modules to place their content programmatically.
My initial feeling was to name it "top", but it could cause confusion with the top bar, so ended up using the "promoted" name for the section and the related hooks. I'm totally open to better naming suggestions.
Regarding the footer, given that's a region that already exists, this change might need more internal discussion. I'm more inclined to leave it out of the scope of this MR for now.
Good catch! SSO failed for any reason and I was not logged in into Gitlab, while I was in d.o.
MR is not ready for review.
Hi @anybody @jsacksick.
Created a new branch for 3.0.x where I cherry-picked the commit in the MR above and worked as expected.
However, I'm getting a 404 error when trying t access to the URL to generate the MR. https://git.drupalcode.org/issue/commerce-3303111/-/merge_requests/new?m...
You can checkout the branch locally and merge it if needed.
Diff can be checked in https://git.drupalcode.org/issue/commerce-3303111/-/compare/3.0.x...3303...
Thank you!
Moving back to active since other issues like 📌 Integrate Navigation with Workspaces Needs work or 📌 Integrate Umami message Needs review
Initial round to define the new TopBar render element and the plugin system to define top bar items.
We can iterate from here if the approach is considered valid.
plopesc → changed the visibility of the branch 3484564-define-top-bar-regions to active.
plopesc → changed the visibility of the branch 3484564-define-top-bar-regions to hidden.
Code looks good and it has nice test coverage. Thanks!
However, it only adds the item to node pages.
We might need to discuss whether we need to extend it to other entity types, as the top bar does. Using NavigationRenderer::meetsContentEntityRoutesCondition() would help.
Besides the entity types, do we expect to have it in other pages to allow to modify header or footer blocks? That's the behavior we have now with the toolbar integration.
Checked the code and in general looks good.
Agree with @ckrina that would be great to move the messages to the top.
I think that would be interesting to provide a simpler way to implement messages by other modules. We can probably discuss it as part of ✨ Allow modules to hook into top of content/footer sections of new core navigation Active .
Thank you for your work on this one @finnsky!
Working on the definition of the 3 new areas.
Thank you @ckrina for the new updated design and explanation of the areas.
Given that top_bar is not intended to be configurable for end users, we agreed that it should be handled using a code only approach, not providing a UI for it.
Navigation Top Bar module will provide a hook, hook_navigation_top_bar(), where modules will be capable to add items to the 3 different top bar regions (tool, contextual & actions). A similar alter hook will allow to modify the items added beforehand.
Navigation Top Bar will provide a render element where modules will declare their Top Bar elements, the section they belong to and their weight, in a similar way as toolbar_item element does.
Contrib module developers will need to be careful adding new elements to this section, given that space is limited and adding more items than necessary could cause negative effects in the site UI.
If "manual" here means "the user has to do it", isn't that quite a regression from what was previously possible with hook_toolbar() and hook_toolbar_alter()?
Yes, I agree.
Difference is that new navigation allows to modify the order of the sections through the UI, and it adds an extra layer of complexity that makes the automatic integration of new elements slightly more complex.
This issue was postponed because we currently have other priorities in the roadmap to convert Navigation into a stable module and integrate it with Drupal CMS. 📌 New Toolbar Roadmap: Path to Beta & Stable Active
A proposal for having 2 "toolbar" like sections for the navigation top and footer could be a possible approach. That would leave only the "content" region managed through LB.
Or provide a simpler API for adding Navigation blocks to the LB during module installation, that could be exported as config afterwards.
Initial approach including base test coverage.
This issue has been discussed internally and for now it seems that we are going to rely on Layout Builder's capabilities to manage this.
✨ Provide Config Action to add new blocks to navigation from recipes Active Will give the possibility to modify the config using recipes, as this is the way to extend layout builder config is expected to use.
3rd party integrations without recipes might need the manual step of adding blocks to the navigation bar once the modules are installed.
Thank you for your review @catch.
Made a couple of small changes from your comments and answered to one of you MR questions.
One thing to check here - does the current implementation result in the render cache for this block caching per-route.
Checked at DB level and we are not introducing new items in the cache_render table related to this block.
Tests are now green. We might need an extra FunctionalJavascript or Nightwatch test to validate the user name is replaced by JS successfully, but I would prefer to have the approval of the JS approach first.
Once 📌 Provide a NavigationLinkBlock Plugin and use Help as an usage example Needs review has been merged, started to work on this one.
Here is a brief summary of the approach taken to be validated.
- Navigation provides a new menu
navigation-user-links - This menu is locked, so it should not be deleted unless unlikely things happen
- Navigation provides default links for the menu mentioned above (Same links we have now hardcoded)
- NavigationUserBlock renders the menu, so new links could be added
- To customize the Navigation, rendering the Username, JS is used to avoid caching issues. Small flickering that might need to be addressed.
- If the Menu is removed for any reason, the navigation item fallbacks to a button that links to the user profile page
Please take a look and validate the approach before continue polishing and updating tests.
Refactored the approach to use OOP hooks instead of EventSubscribers once 📌 OOP hooks using attributes and event dispatcher Needs review is in.
Block looks good and access improvements are great.
Marking as RTBC.
Validation could be a good approach, and necessary.
My concern about validation-only approach is that selected menu could be removed from the system by accident, or deliberately, breaking the navigation. In that case, we might need to provide something.
If the concern is about cache, we could make it in a way where the block would not need to be cached per user, given that links in the fallback would be /user, /user/edit & /user/logout, which are not user-specific.