🇪🇸Spain @plopesc

MR created

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

The suggestion looks valid to me.

Changed settings to #settings and I didn't detect any regression afterwards.

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

Added new approach to try to address this issue.

When the user lands in the Navigation blocks page, there is a button to enable the edition mode. Only when the edition mode is enabled, LB taker over the navigation.

Once changes are saved or discarded, the user is back to normal mode, where the left bar navigation is fully functional.

Thanks! Could this change be backported to 2.x branch?

Update IS.

Describe your bug or feature request.

Follow-up from 🐛 Not compatible with devel module Active , where there is an incompatibility between devel and commerce order item UI modules because devel module is not capable to build the URL properly because the order parameter is missing.

Our suggested approach is to implement the urlRouteParameters method in OrderItem class, in a similar way as other commerce order related entity types do. See Payment, ProductVariation, PaymentMethod or Coupon.

If a bug, provide steps to reproduce it from a clean install.

• Install Commerce Order Item UI module in a vanilla commerce site
• Install devel
• Create an order
• Visit the order page (admin/commerce/orders/X)
• Click on the Order Items tab
• Fatal error is thrown

Thank you for reporting.

Actually that's a missing route parameter issue because commerce does not foresee that order item entity could be exposed.

Opened ✨ Implement urRouteParamenters method in commerce_order_item entity Active to address it from the commerce side.

You can try the patch there to solve this issue.

plopesc → created an issue.

Agree that adding a new UI to handle this could be complex and not easy to maintain, since new modules can add new content entity types.

Besides Gàbor's approach, another possibility could be to use create a theme wrapper for the PageContext render array. That theme wrapper would expose a preprocess hook, from where XB or other modules could alter the PageContext content. That would allow to customize quickly not only the icon, but also the badge text or status.

Back to RTBC after rebase.

It looks great!

Marking as RTBC. Thank you!

Good progress so far! :)

Added a minor comment to the MR to reduce a bit the file footprint. Besides that, would be great if the Navigation settings form page could be updated as well. It is defined in navigation.routing.yml file.

Thank you!

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

plopesc → created an issue.

It looks good!

Will probably need to be updated along the way depending on progress in other fronts.

It's great to have the icon back!

MR rebased. Back to RTBC

Back to RTBC after rebase.

Path looks great and does its job very well.

This is very helpful for pages with multiple exports attachments.

plopesc → created an issue.

Created MR

plopesc → created an issue.

Everything looks good to me!

Marking as RTBC.

Code looks good in general. Default config definition would be needed, though.

Also added a minor suggestion that might be out of scope.

Are tests also considered for this new feature?

Created new MR against 3.x

Update branch

Code looks good, tests are green and the UX of autocomplete widgets is much better now.

Screenshot of the Blog Post form:

I think it covers the agreed objectives for this initial issue. I would mark it as RTBC, but I'd rather to have a second pair of eyes more familiar with the issue actual scope.

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

Created MR. Attached before & after screenshots of the Transaction from.

Before:

After:

plopesc → created an issue.

Branch rebased and MR is green. Back to NR.

MR created.

plopesc → created an issue.

We experienced something similar while integrating Navigation in one of our sites.

There was a global rule to apply specific styles to all the buttons across the whole site that cause conflicts with Navigation.

As reference, this is the diff that made the trick in that project:

-button,
+/* Apply button styles in layout container to avoid global overrides */
+.layout-container:where(button),
 [type="submit"] {
...

Taken the CSS bits from the mentioned issue. Ready for review.

@nod_ if I'm not mistaken, this bug was a regression introduced by 📌 Use a placeholder for the navigation toolbar Active to adapt Navigation to 📌 Render the navigation toolbar in a placeholder Active and looks like that none of them were backported to 11.1.x.

Not sure if we need to backport this one to 11.1.x then.

Thank you for jumping into this @m4olivei!

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

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

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

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

Code looks good to me!

And it seems new bits fond are coming as part of other MRs.

Note for other reviewers: Instead of the 3 MRs approach suggested in #5 🐛 Navigation Top Bar accessibility issues found by Nightwatch tests Active , the lines of code that triggers the error are in MR 11257, which will be always green because the Top Bar is not going to be present since the line to force it have been reverted. Please check the previous pipeline to confirm that worked, or test it locally adding back those lines.
That MR is ready to be merged as it is.

Last code bits have been addressed and tests are green now.

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

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

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

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

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

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

Looks good to me. Thanks!

Thank you for your feedback @berdir.

Moved the post_update hook to the parent module to reduce possible frictions.

Would be great to grant credit in this issue to all the people who worked on the original one and helped to disclose the top bar visibility issues and fix them.
However I'm not allowed to grant credit by myself, I would ask core committers to add credits on my behalf, specially to joaopauloc.dev.

plopesc → created an issue.

Since this is not an easy bug to reproduce, created 2 branches:

• 3508199-navigation-top-bar-axe-test-only: It contains only the code necessary to make the error visible in Nightwatch tests
• 3508199-navigation-top-bar-axe: It should contain the code necessary to make the error visible in Nightwatch tests + the changes to fix it

Once tests in 3508199-navigation-top-bar-axe are green and approved, might be necessary to create a new branch that contains only the code necessary to fix the error. This branch will contain a diff between the 2 original ones, and that's the one that should be merged.

I know this is a bit tricky, but I have not been capable to figure out a better flow for this issue. If someone else have a better proposal, please feel free to show your concerns and ideas.

Postponing on 🐛 Navigation Top Bar accessibility issues found by Nightwatch tests Active since some accessibility issues were found. See https://git.drupalcode.org/issue/drupal-3507866/-/jobs/4431574#L2723

Besides that, there is another Kernel test failing reporting something related to navigation_top bar module tests. Those tests were removed as part of this MR. Tried to reproduce locally and the test is passing. See https://git.drupalcode.org/issue/drupal-3507866/-/jobs/4431573#L659
Any idea about how this test could be addressed?

Add 🐛 Navigation Top Bar accessibility issues found by Nightwatch tests Active

plopesc → created an issue. See original summary → .

Created MR that removes the checks for the module flag and marks it as obsolete.

Added a couple of questions to the MR about the bureaucracy of the process of marking the module as obsolete.

Would be great to have this enabled by default just removing the flag module.

Just a couple of questions.

For sites where the flag module is already enabled, how should we proceed? Just removing the module would cause an error. On the other hand, this was an alpha module within an experimental one, so people who enabled it should be able to manage this scenario.

Navigation is used in Drupal CMS, where Gin Toolbar is enabled as well. Removing the flag would automatically enable both top bars for sites using the 1.x branch, I think.

These 2 questions, and maybe others I missed, make me think that we might need a 2 steps path.

LGTM! RTBC +1

Bumping 📌 Adjust how Help link and Content links are removed from the Administration menu for the Navigation bar RTBC to Stable Blocker.

Bunmping it to stable blocker since this bug could cause unexpected behaviors in contrib modules. Found it problematic with Recurring Events.

Thank you! Just a minor comment regarding a redundant permission and this one should be ready to go :)

MR rebased and 3rd party tests included last weeks updaed accordingly. It is green now.

The alter hook suggested by catch was already implemented.

Also found that it can cause issues with contrib modules declaring menu items depending on the `system.admin_content` menu link.
Hence bumping the priority and marking it as a Bug.

Thank you for jumping into this.

Left some comments in the MR.

Went through the proposal and looks great to me.
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.

While testing this, found an issue that might need to be considered in this MR.

Environment Indicator provides its own top bar when detects that Toolbar integration is not present. That leads to unexpected behaviors when toolbar module is disabled or its integration disabled.

1. Install Drupal 11.x using standard profile
2. Install Navigation and Environment Indicator modules
3. Uninstall Toolbar
4. Add the Environment Indicator Navigation block to the sidebar

Actual behavior:

The extra top bar is unnecessary, but there is no way to get rid of it, since it is hardcoded in hook_preprocess_html() and hook_page_top().

For the hook_navigation_content_top implementation could be simple to just not add this top bar when Navigation module is enabled and its integration enabled. However, for the navigation sidebar block based approach, it's not trivial to found whether the block has been added to the sidebar, even if possible.

We're working on these tests in 🐛 Navigation Expand/Collapse logic is not working properly in conjunction with Big Pipe Active . If finally approved there, this one could be closed.

Rebased and made some adjustments. I think it needs a new round of reviews to ensure there are no regressions.

Minor suggestion about the usage of the class constant instead of the hardcoded value.
Test needs to be adjusted to reflect the new cache tag.

If I followed the reasoning correctly, that would imply to add a new prop or slot to the navigation:toolbar-button component.
Not sure if that would increase the component complexity more than we would like to in edge case scenarios where both title and button_label properties are set.

Back to the proposal, being able to make it happen at that level would simplify the overall logic. If that does not generate any undesired side effect, looks promising.

An extra pair of eyes from a more experienced FE would help to have a better idea of the pros and cons.

The comment related to the FunctionalJavascript test has not been resolved or implemented, so might need to be checked explicitly.
I think there was a misunderstanding related to the usage of static::NAVIGATION_LINKS_MENU, probably I was not clear enough in my original comment.

Created MR 11227 that demonstrates that test-only changes break the Nightwatch test, while the original MR 11124 is still green.

Made some new changes in the Nightwatch test to ensure that cache is cleared before reloading the navigation after installing bigpipe and it failed locally without the JS changes.

However, the test-only job stays green. I don't see whether the specific nightwatch test was executed either. Could it be something wrong with the CI job?

Tested manually and it looks very good after last changes.

We might need some test coverage to consider this one completed, though. This test could be inspired in Drupal\Tests\contextual\FunctionalJavascript\EditModeTest, where the toolbar integration was tested.

Besides that, we need to consider a couple of things:

• Eye icon is not being visible in latest chrome version. See 🐛 The icon for the more actions button is not visible Active
• Icons are being migrated to Icon API 📌 Navigation leverage icon core API Needs work . Might need to take this into account and migrate the eye icons to this new approach if that issue lands first

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

plopesc → changed the visibility of the branch 3465295-integrate-top-bar-contextual-links-preview to hidden.

The idea is great and the code in the MR looks sensible to me.

However, I have not been able to find a way to make it work. This is probably out of my limited FE knowledge.

Adding some debugging information that could be useful for a FE development who could take a look into this:

Replace code in NavigationRenderer::buildNavigation() by something similar to this:

  public function buildNavigation(array &$page_top): void {
    $page_top['navigation'] = [
        '#type' => 'html_tag',
        '#tag' => 'aside',
        '#attributes' => [
          'class' => ['admin-toolbar'],
          'id' => 'admin-toolbar',
        ],
        'child' => [
          '#type' => 'container',
          '#attributes' => [
            'class' => ['admin-toolbar__displace-placeholder'],
          ],
        ],
    ];
  }

When any page is loaded, the space for the navigation bar should be reserved, but empty.

Once that behavior is achieved, that chunk of HTML should be moved to the '#lazy_builder_preview' render array property in the original output.

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

plopesc → created an issue.

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

Just mentioning a few things to take into account:

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

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

Approach looks great and would simplify the code significantly if we can live with the extra cache items generated.

Those extra cache elements were my main concern to try to avoid caching per user initially. Since we are using the new placeholder approach, only the use menu bits are being cached per user, so seems a good balance.

Added a few comments to the MR, also a test might need to be updated to adapt it to the new cache tags.

IS might need to be updated as well, the current approach differs from the original one.

Thank you for the clean up and taking care of the unused dependency. That was my fault, I think.

From my perspective, it can be marked as RTBC.

Updated the issue summary to reflect the last requirements from a11y team.

Tested locally and worked as expected.

Fixed the Kernel tests failing due to conflicts between DomDocument::loadHtml and HTML5 tags (https://www.php.net/manual/en/domdocument.loadhtml.php)

I think this is ready for final review.

Tested locally disabling the views mentioned in the IS and test coverage is great.

Not only adds test coverage for the reported bug, but also for the rest of the logic in the NavigationContentLinks class.

Checked the code and looks good to me. Added a couple of comments in the MR that might help to clean it up a bit.

Agree that a possible 3rd color for this new status might be a good addition, but we could use this MR as an intermediate step while the UX and design teams come up with a final solution.

Thank you!

Tested manually and it behaves as expected, fixing the reported bug.

However, the JS here is out of my technical knowledge. So I prefer to have the RTBC signoff from a more experienced FE developer.

Thank you!

Created MR where the mentioned if clause is replaced by a check for the element in the context.
Also updated the specific Nightwatch test for this functionality to use big_pipe.

Tested manually and at first glance it works well, but a review from more experienced JS developer would be appreciated.

Thank you for reporting!

Filled 🐛 Navigation Expand/Collapse logic is not working properly in conjunction with Big Pipe Active to keep track of it.

plopesc → created an issue.

They're not directly related to caching, but the way it was supposed to cache the navigation HTML markup in local storage to not serve it in every request was not compatible with having IDs in the navigation HTML markup.

All the discussion and the reasoning is available in 📌 Implement a caching strategy for the menu links Active .

The

IDs were removed in https://git.drupalcode.org/project/drupal/-/merge_requests/7980/diffs?co... and they need to be brought back using JS for a11y reasons.

The

Those IDs can still be there, since we are changing the way to improve the Navigation performance.

Made a rebase once 📌 Use a placeholder for the navigation toolbar Active was merged and addressed the last comment in the MR.

I think we can close this one.

The scenario reported in the issue is already covered in NavigationUserBlockTest::testNavigationUserBlock().

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

Created 📌 Add tests to ensure that Navigation Top Bar Page actions respect the original local tasks weight Active to take care of the missing test.

plopesc → created an issue.

Great job with the tests! Added some comments to the MR to polish them a bit.

Worked on the bits mentioned in the MR. I think this is ready for a new review.

I already made the Navigation changes in the old MR that was replaced by the current one, so I could easily cherry-pick them. On the other hand, keeping the focus here would make this issue simpler.

I'm totally OK if you feel that's better to have methods separated, just wanted to reduce a bit the amount of code, but the downsides could be higher than the benefits.

Regarding docblocks, I can take a look there, current ones were just a copy&paste from the suggested in #12.

MR created. It still need some tests, though.

Checked the codebase and found out that NavigationContentLinks class does not have test coverage yet. This issue could be a good opportunity to add a new NavigationContentLinksTest class and test the behavior of the menu modifications made by Navigation module.