- Issue created by @m4olivei
- 🇨🇦Canada SKAUGHT
Navigation project goals include: ✨ Add the user image to the user menu when available Active
How User menu is made: User menu is provided by a Menu Plugin (YAML FORMAT)
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/user/...user login/logout item:
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/user/...
items itself dynamically changes title & path. cache content 'user.roles:authenticated'----
Perhaps it's simply that using the same namespace 'user' should be renamed to keep more separation for when a site may want to place the traditional list of Account Items?#[NavigationBlock( id: 'user', admin_label: new TranslatableMarkup('User'), )]
- 🇬🇧United Kingdom catch
If this needs to be different to the user account menu that would normally be in the front end theme (brought up by @ckrina on the MR), then it might also be possible to define an additional menu like 'Navigation user links' so that they're separate - but that would still allow people to add an extra link or remove something if they want to.
- 🇪🇸Spain ckrina Barcelona
Agreed. I missed the part about customizing the links. Tagging this as Stable Blocker.
- 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland
Having the "User account menu" and the user account links that appear in the toolbar being different does feel like a weird quirk, from a UX perspective I think using the "User account menu" and "modernizing" it makes a lot of sense. As noted, it also allows the user menu links in the toolbar to be customized, which is a win in my opinion. I think it would be weird if every other menu in the Navigation could be customized apart from the user links.
- 🇬🇧United Kingdom catch
There are sites that customize the user account menu links with links that they might not want in the navigation toolbar, however, if it's just a menu block in the layout, those sites could swap it out with a different menu block easily enough. So using the same block by default might be plenty?
- 🇬🇧United Kingdom AaronMcHale Edinburgh, Scotland
however, if it's just a menu block in the layout, those sites could swap it out with a different menu block easily enough. So using the same block by default might be plenty?
Agreed. We should think in terms of what's the most sensible default for the 89% use-case. As you say, they can always use a different menu on the front-end, or swap this one out for a different one in the navigation.
- 🇪🇸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
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 aboveNavigationUserBlock
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?
- Navigation would provide a Menu
- 🇬🇧United Kingdom catch
#14 looks good, with the slight caveat that I'm not sure about the fallback - could that be handled with validation maybe?
- 🇪🇸Spain plopesc Valladolid
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. - Merge request !9904Issue #3441576: Investigate using the core "User account menu" in favor of... → (Open) created by plopesc
- 🇪🇸Spain plopesc Valladolid
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.
- Navigation provides a new menu
- 🇬🇧United Kingdom catch
Couple of small comments on the MR. More general thoughts here:
- I think an extra menu is a good idea, allows it to be customized separately from any visitor-facing user menu.
- 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 very minimal fallback added here seems fine.
- 🇪🇸Spain plopesc Valladolid
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.
- First commit to issue fork.
- 🇬🇧United Kingdom catch
One thing to check here - does the current implementation result in the render cache for this block caching per-route. If so, it would be good if we're able to avoid that. The main reasons for per-route caching are the active trail for expanded menu links and maybe the 'active' class.
- 🇷🇸Serbia finnsky
js not works
https://gyazo.com/f93eb21f5e7ddba4797e2cc9292d056f - 🇷🇸Serbia finnsky
i fixed js but seems we need to care about reload again
https://gyazo.com/e1a88c9d32cbd215f97a11b6198d92b6also keyboard search broken because button still has data-index-text="u". not sure it is critical. maybe we also need to move that functional out of twig to js
- 🇪🇸Spain plopesc Valladolid
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.
- 🇨🇦Canada m4olivei Grimsby, ON
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?
- 🇪🇸Spain plopesc Valladolid
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.
- 🇪🇸Spain ckrina Barcelona
Adding the "Drupal CMS release target" to see if we could hopefully get to it.