🇪🇸Spain @plopesc

@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.

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

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.

plopesc → changed the visibility of the branch 4.3.x to hidden.

plopesc → changed the visibility of the branch 4.2.x 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.

plopesc → created an issue.

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. 🌱 [PLAN] 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.

Postponing on ✨ Create Javascript library for searching rendered lists on the client. Needs work

Now that 📌 Provide a NavigationLinkBlock Plugin and use Help as an usage example Needs review is closer and we are pushing again towards Navigation stability, I would like to make a proposal for this one:

• Navigation would provide a Menu Navigation User Links, or a better name to be proposed
• That Menu would include by default the same links we currently have hardcoded.
• NavigationUserBlock class will give the ability to choose a menu to render as user links. By default we would use the Menu provided above
• NavigationUserBlock would render by default the links from the menu stored in config. If empty, or the Menu does not exists, it would fallback to the hardcoded links we have now. In this way we will ensure that removing the menu will not lead to unexpected behaviors

Would this proposal cover all the previously mentioned needs?

Thank you for your review @catch.

Made a refactor of the validation and error handling logic. Hope it is good to go now.

Thank you for the review and summary update.

Answered the outstanding question.

This will be probably necessary for Starshot.

Thank you for your review @penyaskito.

Made some adjustments in the MR according to your comments.

We agreed to create an scaffolding recipe for he Navigation Track during the kickoff call we had on 11/10 with Cristina and Tim.

The aim was to provide some scaffolding that would allow to provide more complex navigation configs in the future.

If you feel that we should reuse the existing drupal_cms_admin_theme, that would work for me.

In that case, renaming it to drupal_cms_admin_ui would be more appropriate, I think, given that Navigation bar is not present only in the admin theme, but also in the main theme.

penyaskito → credited plopesc → .

Recipe created and tests are green. Ready for review.

plopesc → created an issue.

Moving to NR

plopesc → created an issue.

MR is back to green after merging 11.x again.

plopesc → created an issue.

Added steps to reproduce and MR is still valid. Moving to NR

MR updated to solve the PHPStan and PHPCS errors after ✨ Enforce return types in all new methods and functions RTBC

Great step forward @penyaskito. Really looking forward to have this in core!

Tried to test it locally to integrate it with navigation module with no luck. Probably I missed one or more steps.

Steps I followed:

• Clone the branch
• Add use ConfigSectionListTrait; to NavigationSectionListStorage
• Implement uuidGenerator() method
• Clear cache at least once
• Execute ddev . php core/scripts/drupal recipe modules/my_recipe
• Get the error: The "addComponent" plugin does not exist. Valid plugin IDs for Drupal\Core\Config\Action\ConfigActionManager are: ....

Create a recipe.yml like this:

name: Navigation Test Recipe
type: Examples
description: 'Adds a block to navigation.'
install:
  - navigation
config:
  actions:
    navigation.block_layout:
      addComponent:
        section: 0
        position: 1
        value:
          uuid: 30000000-0000-1000-a000-000000000000
          id: 'navigation_menu:content'
          label: Content From Recipe
          label_display: 1
          provider: navigation
          region:
            content: 'content'
          default_region: content
          level: 1
          depth: 2

Could you please indicate the steps I missed?

Thank you!

I share the concerns mentioned above and we might also need to include another edge case to the equation to be considered:

- User modifies its own password while the site is in WASM mode. It would require to capture somehow the password and update the block accordingly.

Hello all,

I don't actually understand the aim of removing the redirect feature from Dashboard module to replace it with ECA redirects.

ECA is a great module and helps to fill the gaps and build new features in an easy and approachable way for non-devs. Also devs can benefit of it, investing their time in more complex tasks.

Dashboard initiative members have been working on it for more than 2 years, conducted different user studies together with the rest of the Admin UI improvements team. The features provided by the module have been already proved as improvements that would help to the Drupal ecosystem.

In the context of Drupal CMS, we have ECA, and redirects could be managed using that module. However, not all the Drupal sites rely on ECA, so the redirects provided by Dashboard module are helpful for them.

Using the Dashboard provided redirects, the amount of code to maintain as part of Drupal CMS team will be smaller, and the ECA team could focus on new tasks that could provide an actual benefit for the project.

In my opinion, we could invest the efforts put on this discussion that is transparent for the average Drupal CMS user, as they might not be interested on whether the redirect is coming from one module or the other, in other tasks where the community could benefit.

Thank you.

@alexpott I'm fine leaving it as it is unless you have special concerns about it.

Should we publish the CR once the issue has been fixed? Or should we wait until 11.0.6 is released?

MR refactored once 📌 Reorganize navigation settings to be more consistent RTBC has been merged.

CR created and addressed question about config structure.

Created new MR !27 adding exceptions for destination query parameter and one time login links.

It has been interesting because in order to make test compatible with both D10 & D11, we had to add some tricky logic due to ✨ Use one-time login link instead of user login form in BrowserTestBase tests Fixed .

Hope once this is merged the Drupal CMS repository could rely on these redirects instead of custom ones.

Thank you!

Thank you for your review @alexpott.

Made the changes requested and tests are still green. Took the freedom to move it back to RTBC.

Thank you for bringing up that issue, I was unaware of it. Definitely avoid reinventing the wheel should be a priority.

Navigation use case might differ a bit, given that sections and regions are more limited than in other scenarios. I'm pretty sure we could take advantage of that trait, though.

This initial approach is a proof of concept to validate the approach. More than happy to readjust it to be based on the code in the mentioned issue once it is ready.

Thanks!

Not sure if the aim of this issue is exactly the same, but discussion about Config Actions for Navigation has been opened in ✨ Provide Config Action to add new blocks to navigation from recipes Active .

First shot for this one.

It allows to provide a recipe like this:

name: Navigation Block Test Recipe
type: Examples
description: 'Adds a block to Navigation.'
install:
  - dashboard
config:
  actions:
    navigation.block_layout:
      addNavigationBlock:
        position: 1
        configuration:
          id: 'navigation_menu:content'
          label: Content From Recipe
          label_display: 1
          provider: navigation
          level: 1
          depth: 2

After applying the recipe, a new block with the settings described above is added to the Navigation.

This one needs tests and some extra work, but let's open the discussion about it.

plopesc → created an issue.

The problematic route entity.commerce_order_item.devel_load is not provided by this module.

Could you please confirm the module that provides the route is working as expected or requires anything special?

It seems devel module should provide some kind of error handling.

Thank you.

Attaching screenshot of the new Navigation settings UI.

Created a new MR for this one where the approach is more similar to /admin/appearance/settings.

Site administrator can enter a path or upload an image whose path will be automatically used.

MR created.

plopesc → created an issue.

MR merged. Thanks!

Thank you @cleavinjosh!

Changes look good.

Took a look and code looks great!

Neat and simpler than I expected.

Code looks great.

Just a minor nitpick and PHPCS cleanup are needed before merge.

Thank you!

MR merged! Thank you

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