Investigate using the core "User account menu" in favor of custom Navigation Block for same

Created on 17 April 2024, 8 months ago

Problem/Motivation

In the current version of the module, the user Navigation block that lives at the bottom of the Navigation bar is a set of custom links defined in \Drupal\navigation\UserLazyBuilder::userOperationLinks. It is not using the "User account menu". This was brought up as a question as to why we have this approach, by catch in the core MR ( Add the new Navigation to core as an Experimental module Fixed ). Link to the specific, relevant comment:

https://git.drupalcode.org/project/drupal/-/merge_requests/7474#note_297387

It appears the current implementaion has been carried as is, since early in the development lifecyle and this question hasn't had sufficient scrutiny.

A couple things that would be different were we to use the "User account menu", is we would loose the "Edit profile". The default label would be different, we currently have "View profile", core has "My account".

The task here is to investigate:

  • Do we require an "Edit profile" link, or would it be OK to loose that in using the "User account menu "
  • Are there caching concerns of current approach vs using core menu
  • Is there a substantive reason why we need to keep the current approach, vs using the "User account menu"
📌 Task
Status

Active

Version

1.0

Component
Navigation 

Last updated about 18 hours ago

No maintainer
Created by

🇨🇦Canada m4olivei Grimsby, ON

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @m4olivei
  • 🇨🇦Canada m4olivei Grimsby, ON
  • 🇨🇦Canada m4olivei Grimsby, ON
  • 🇨🇦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.

  • 🇪🇸Spain ckrina Barcelona
  • 🇬🇧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 ckrina Barcelona
  • 🇪🇸Spain plopesc Valladolid
  • 🇪🇸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 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?

  • 🇬🇧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.

  • Pipeline finished with Failed
    2 months ago
    Total: 103s
    #316978
  • Pipeline finished with Failed
    2 months ago
    Total: 136s
    #316990
  • Pipeline finished with Failed
    2 months ago
    Total: 563s
    #317007
  • 🇪🇸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.

  • 🇬🇧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.

  • Pipeline finished with Failed
    2 months ago
    Total: 356s
    #317262
  • Pipeline finished with Failed
    2 months ago
    Total: 266s
    #317330
  • Pipeline finished with Canceled
    2 months ago
    Total: 843s
    #317342
  • Pipeline finished with Failed
    2 months ago
    Total: 430s
    #317351
  • Pipeline finished with Canceled
    about 2 months ago
    Total: 379s
    #318258
  • Pipeline finished with Failed
    about 2 months ago
    Total: 1256s
    #318268
  • Pipeline finished with Success
    about 2 months ago
    Total: 1013s
    #318297
  • 🇪🇸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.
  • Pipeline finished with Failed
    about 2 months ago
    #318640
  • 🇬🇧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

    i fixed js but seems we need to care about reload again
    https://gyazo.com/e1a88c9d32cbd215f97a11b6198d92b6

    also 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

  • Pipeline finished with Failed
    about 2 months ago
    #318658
  • Pipeline finished with Failed
    about 2 months ago
    Total: 201s
    #322833
  • 🇪🇸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.

  • Pipeline finished with Success
    about 2 months ago
    Total: 696s
    #323218
  • Pipeline finished with Success
    about 1 month ago
    Total: 759s
    #344916
  • 🇨🇦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.

Production build 0.71.5 2024