- ๐ฎ๐ณIndia Kanchan Bhogade
Hi
I've tested MR !8117 on Drupal 11
The MR is applied cleanly...Special Menu items' empty links in the navigation issue is fixed.
RTBC+1
Adding SS
- ๐ช๐ธSpain plopesc Valladolid
Adding ๐ Agree on a lazy load strategy Active as reference to deal with caching at user level issues.
My concern or question about how to integrate a menu here is because most of the links I think that could be added to this menu are user specific, like
/user/XX/foo
and I am not sure how we could handle routes like that from a menu.Mostly thinking about menu links created from the UI by site admins, where it is not possible to indicate URL parameters.
Core's user routes provides shortcuts like /user or /user/edit that redirects to the user specific page, but that's something not all the contrib modules that provide user specific routes or views that use the user ID as parameter would support.
We could encourage the usage of modules like https://www.drupal.org/project/user_current_paths โ or https://www.drupal.org/project/me_redirect โ for this menu, though.
- ๐ช๐ธSpain plopesc Valladolid
Thank you for the heads up!
Planned to deal with this as part of the mentioned issue ๐ Investigate using the core "User account menu" in favor of custom Navigation Block for same Active once the related issues ๐ Provide a NavigationLinkBlock Plugin and use Help as an usage example Needs review & ๐ Rename UserNavigationBlock to NavigationUserBlock for class name consistency RTBC are in and the ground is ready to mess with it.
We can leave this open as a reference and reminder in the meantime, though.
- ๐ท๐ธSerbia finnsky
I don't think proof of concept should be rebased. But thank you anyway.
- ๐ฎ๐ณIndia Mithun S Bangalore
Mithun S โ made their first commit to this issueโs fork.
- ๐ฎ๐ณIndia Gauravvv Delhi, India
Gauravvvv โ made their first commit to this issueโs fork.
The Needs Review Queue Bot โ tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide โ to find step-by-step guides for working with issues.
- ๐บ๐ธUnited States smustgrave
Can we had a test case to cover this scenario please
- ๐ฌ๐งUnited Kingdom catch
Committed/pushed to 11.x and cherry-picked to 11.0.x/10.4.x/10.3.x, thanks!
- ๐ท๐บRussia kostyashupenko Omsk
Good catch.
It's now ready for review.
made also span element not interactable visually (no hover styles). Also added few fixes for forced-colors: active mode - @kostyashupenko opened merge request.
- ๐จ๐ดColombia jidrone
Hi everyone,
I have been thinking on this from the DrupalCon Portland after the conversation with @ckrina and lauriii, this is my proposal:
- Create a module called Icon Library:
- Detects svg sprites or single icons from any module or theme.
- Provides field, widget and formatter.
- The widget should be a button to open the icon library.
- The icons should use a naming convention to provide context that can be used as filters, that will avoid storing metadata for the icons in database or config, the pattern could be: solid-megaphone, outline-megaphone, duotone-megaphone
- A filter for the Icon library by Provider - The module or theme that provides the icons - FontAwesome, Claro or any other module or theme.
- A filter for the Icon library by Style - Solid, Outline, Duotone or any other word extracted from the first part of the name of the icon.
- The icon library should provide searchbox to find icons by name.
- The value for configuration or content entities should be the icon id
Let me know what do you think?
- ๐จ๐ฆCanada SKAUGHT
Certainly!
It is true, unless I add an item the main branch item doesn't appear.๐ Navigation glitch with Shortcuts and Minimal Profile Active have opened for followup.
- ๐ช๐ธSpain plopesc Valladolid
Thank you for your review!
I think the scenario described in your manual test is missing the step to create shortcut links.
When there are no shortcut links to show to the user, the shortcuts navigation block is not rendered.
According to https://git.drupalcode.org/project/drupal/-/blob/11.x/core/profiles/stan.... Standard profile creates some shortcut links by default during installation.
It is possible that this is not happening in minimal, where you need to create some shortcut links by yourself to have access to the shortcuts block.
Could you please repeat your tests and confirm this?
Thank you!
- ๐จ๐ฆCanada SKAUGHT
After installing Minimal profile, then going to admin/extend to enable Navigation (alone) it is initializing without error. As we did not enable Shortcuts the item is not there (so far as expected!).
-next, enable shortcuts module. notice: no "(star) shortcuts" appears at top of all others.
- clear cache. does not "(star) shortcuts" item to appear.
-next step to try: disable navigation. LEAVE Shortcuts ENABLED!.
- clear caches.
- re-enable navigation. still no "(star) Shortcuts" item.
---
meanwhile: reinitialize env -- install regular standard profile, then enable navigation -- "(star) shortcuts" is working as expected. - ๐บ๐ธUnited States smustgrave
smustgrave โ changed the visibility of the branch 3445184-fatal-error-when to hidden.
- ๐ฌ๐งUnited Kingdom catch
*/ public function build(): array { return [ 'user' => [ '#lazy_builder' => [ 'navigation.user_lazy_builder:renderNavigationLinks', [], ], '#create_placeholder' => TRUE, '#cache' => [ 'keys' => ['user_set_navigation_links'], 'contexts' => ['user'], ], '#lazy_builder_preview' => [ '#markup' => '<a href="#" class="toolbar-tray-lazy-placeholder- link"> </a>', ], ], ]; }
I think there's still an issue here - it now looks like the preview isn't user-specific, but the caching of the preview still is. This could be verified by using the navigation with a couple of different users then checking the cache IDs in the cache_render table.
It could end up being a duplicate of ๐ Investigate using the core "User account menu" in favor of custom Navigation Block for same Active maybe though.
- ๐ฌ๐งUnited Kingdom catch
#17 is pretty convincing to me that we can do without the ID attributes.
- Issue created by @plopesc
- ๐ช๐ธSpain plopesc Valladolid
During Drupalcon sprints worked on an approach where ID attributes, and all the references to them are removed from the HTML markup before generating the hash, so the problem is gone, but at the cost of some extra calculations that would be great to avoid.
Would be great to have some extra feedback about whether we can get rid of id attributes or should we explore other paths assuming that IDs could be there.
On the other hand, if we remove IDs from the core templates, but a specific site overrides the template making use of them, the navigation would be broken.
That's something that already would happen with current toolbar, but need to confirm if that scenario should be supported by navigation.
- ๐ช๐ธSpain plopesc Valladolid
We can close this given that ๐ Regression: Shortcuts menu flickers when the page is loaded Fixed showed the way to avoid flickering with no need to make an extra effort at render array level.
Same approach could be followed in the future if necessary.
Talked to @mherchel in the weekly Drupal Admin UI call and he also agreed.