Use a placeholder for the username in the navigation user menu

Postponed on 📌 Investigate using the core "User account menu" in favor of custom Navigation Block for same Active .

Tried and failed to use a placeholder for just the name, nowhere to fit the render array into the menu link structure. We could try to make all of that more flexible but not really sure where to start.

But with CachedPlaceholderStrategy we can use the same technique for the whole user menu as we do for the navigation block itself - placeholder it, but also cache it (per user).

When it's in render cache it comes from straight from that without big pipe, which means no flickering at all now. When it's not cached, big pipe will replace it.

Allows the custom js to be removed.

It would be nice if there was a way to use a plugin method as a string for a #lazy_builder callback, but I can't see a way to do that.

We could create an entire new service instead of having the static method maybe and call a method on that.

Ready for review now I think.

Cache IDs for the navigation user block look like this:

| navigation_user_block:[languages:language_interface]=en:[theme]=olivero:[user.permissions]=is-admin                                                                            >
| navigation_user_block:[languages:language_interface]=en:[theme]=olivero:[user]=1  

Cache storage requirement shouldn't be much because even with 100 users actively using the navigation that's only 100 unique cache items.

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.

I think we could placeholder only the username if it wasn't a menu link at all, but we'd lose the fallback menu item logic then. Maybe we could open another follow-up to look at finding a way to do it still, but yeah I think given it's just this block now, caching per user isn't too bad. The navigation itself will still load instantly as soon as there's been a single request for it, the user block will be big-piped in per-user then instant.

Addressed the comments on the MR I think.

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.

Since the custom JS approach is being removed as part of this MR, Drupal\Tests\navigation\FunctionalJavascript\NavigationUserBlockTest could be removed as well, I think.

I think this is probably still useful - makes sure the cache contexts are correct at least.

I'm wondering if we could make a username-only placeholder work after all.

    {% if menu_level == 0 %}
      {% if item.below is empty %}
        <li id="{{ item_id }}" class="toolbar-block__list-item">
          {% include 'navigation:toolbar-button' with {
            attributes: item_link_attributes.setAttribute('href', item.url|render|default(null)). setAttribute('data-drupal-tooltip', item.title).setAttribute('data-drupal-tooltip-class', 'admin- toolbar__tooltip'),
            icon: item.class|clean_class,
            html_tag: item_link_tag,
            text: item.title,
            modifiers: [
              'collapsible',
              item_link_tag == 'span' ? 'non-interactive' : null
            ]|filter(v => v is not null),
          } only %}
        </li>
      {% else %}
        <li id="{{ item_id }}" class="toolbar-block__list-item toolbar-popover" data-toolbar-     popover>
          {% include 'navigation:toolbar-button' with {
            action: 'Extend'|t,
            attributes: create_attribute({
              'aria-expanded': 'false',
              'aria-controls': item_id,
              'data-toolbar-popover-control': true,
              'data-has-safe-triangle': true,
            }),
            icon: item.class|clean_class,
            text: item.title,
            modifiers: [
              'expand--side',
              'collapsible',
            ],
            extra_classes: [
              'toolbar-popover__control',
            ],

When item.below is set, the template extracts the link text for the button.

If we added an additional button_label variable for use only in that branch of the template, could we use that instead? And then that would need to support button_label being a render array so it can include the #lazy_builder etc. Not sure how compatible that is with SDCs but it seems feasible.

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.

If I followed the reasoning correctly, that would imply to add a new prop or slot to the navigation:toolbar-button component.

Either that or preparing the 'text' prop before it's passed into the SDC, but we're beyond the limit of my understanding of SDC conventions and limitations so yes someone who's more on top of that would be great to hear from.

This would allow us to completely drop the separate caching if it works. I'm not sure if it's strictly necessary, what's in the MR should be quite efficient as it is, just it would be even more efficient.

When I started looking into the feasibility of this I was wondering about adding the capability generically to menu links, which could be generically useful for some use-cases although probably harder. This is less generically useful but also probably easier. We could also just go ahead more or less with what's here and open another follow-up to explore.

Back to needs review for the current iteration - I left the test in (see comment above).

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.

Adding a couple of tiny adjustments to fix a docblock comment, align with other instances of specifying a #lazy_builder callback and a small cleanup now we're getting rid of user-block.js.

Otherwise everything looks good, and works well. I've tested with and without big_pipe. I've also tested the fallback by manually removing all the links from navigation-user-links menu and manually deleting that menu.

I believe I've addressed all the open threads, which were all minor cleanup / code style adjustments. Ready for review.

Looks like it needs another rebase, changes since I last looked all look good.

Rebased. Also noted a test failure in NavigationShortcutsBlockTest that was checking cache tags and needed to be updated. I've fixed that, and now tests are passing. Should be good here!

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

merge conflict after 📌 Navigation leverage icon core API Needs work

Fixed the merge conflict. Very straightforward. There was a change made and then removed to core/modules/navigation/js/user-block.js on 11.x. Here we just don't need that script anymore. Fixed the conflict by deleting it.

Since the merge conflict was simple and tests are green, I'm going to go ahead and set this back to RTBC. Feel free to nudge back if necessary.

nod_ → closed merge request !11215

I like removing JS files :)

Committed 73ff0d1 and pushed to 11.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.