Special Menu items are rendered as empty links in navigation
Closes #3447837
Merge request reports
Activity
added 1 commit
- a88e0dfb - Special Menu items are rendered as empty links in navigation
added 24 commits
-
490743ab...ca4e4a81 - 23 commits from branch
project:11.x - 28cfef19 - Merge remote-tracking branch 'origin/11.x' into 3447837-special-menu-items
-
490743ab...ca4e4a81 - 23 commits from branch
78 78 color: var(--admin-toolbar-color-gray-990); 79 79 } 80 80 81 span.toolbar-button, -
i would like to avoid specificity weight grow without good reason here
we probably need to write more universal css here. or reuse class.
Edited by Ivan Berdinsky 
@finnsky How do you think we can address your concerns regarding these CSS changes?
changed this line in version 8 of the diff
25 24 {% if has_safe_triangle %} 26 25 <div data-toolbar-popover-safe-triangle></div> 27 26 {% endif %} 27 {% endset %} 28 29 {% set attributes = attributes.setAttribute('data-index-text', text|first|lower) %} 28 30 29 </{{html_tag|default('button')}}> 31 {% if menu_link %} 32 {{ link(toolbar_content, href, attributes.addClass(classes)) }} 33 {% else %} 34 <{{ html_tag|default('button') }}{{ attributes.addClass(classes) }}> -
html_tag was added here to make option to choose between link or button. why it not works now? it can be choice between button link or span.
we want to move components (and this is best candidate) to SDC. We need to keep things universal and simple.
this component shouldn't contain anything related to
menu_linkor something to be good component.Edited by Ivan Berdinsky -
Agree that we should try to be as agnostic as possible and try to avoid the usage of anything the could tie the component to menus.
However, I believe that using ´link()` function here is a good choice because it already takes care of all the possible ramifications we can find when rendering a Drupal Url object. Trying to do it in twig would be very complex and reinventing the wheel. See TwigExtension or Link.
You know better than me what's the plan for moving this into components and how this would fit.
Said this, it seems that
menu_linkis just a flag set atnavigation-menu.html.twig. Could we just rename that variable to be less confusing? 
Said this, it seems that menu_link is just a flag set at navigation-menu.html.twig. Could we just rename that variable to be less confusing?
I would say this is a good idea.
However, I believe that using ´link()` function here is a good choice because it already takes care of all the possible ramifications we can find when rendering a Drupal Url object. Trying to do it in twig would be very complex and reinventing the wheel. See TwigExtension or Link.
Totally agree. However, I see @finnsky's point here.
linkis a stateful function, and components are best staying stateless. If you think the component should support all the link features (ajax, arbitrary attributes, etc.) then maybe your component's input is not as structured as initially planned. Perhaps you want to make a slot for this, although that's a heavy handed solution.I tend to be flexible about these things. References to the data model, for instance, would be a no-go for me. In this case, I could stomach it given the benefits. But in this case you will be setting an example, so that changes things a bit
-
Thank you for your input @e0ipso!
It seems that there is no clear idea on how to address this issue within the context of SDC and we are at a standstill.
Could we rename the flag
menu_linktolinkto be less controversial and continue with the rest as it is? When the time comes to migrate to components, we will have a clearer idea on how to resolve this situation.How do you feel about it @finnsky?
linkis a stateful function, and components are best staying statelesIndeed,
linkfunction must be avoided in component template, because it may do a query to Drupal API and applicative state:- it may call Url::fromUri(): https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Core/Template/TwigExtension.php#L252
- Url::fromUri() may call services like routing, access checker, path validator... https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Core/Url.php
It is better to resolve all this before sending them to component templates or, as Mateu said, send a renderable into a slot.
Edited by Pierre Dureauchanged this line in version 6 of the diff
Hey all. I took a crack at a solution here to satisfy the concerns. Key to the issue here I think, is that we're really wanting to just extract the appropriate tag given
item.urlso we can pass it along to what will be a stateless SDC (toolbar-button.html.twig). There is one other place I found in core that has a similar need. Given that, I figured, lets extract that logic into a Twig function. Which I've done, so that now you can dolink_tag(item.url)and get back the tag per the logic in Drupal core which determines that. I've adjusted the other place in core to use it, as well as used it for our needs here. Perhaps thats out of scope and should be handled separately. I'm open to that.Open to feedback, or just reverting one or both of the commits.
Edited by m4olivei
added 27 commits
Toggle commit listadded 8 commits
-
ad8c8de9...a220b008 - 2 commits from branch
project:11.x - 405d069f - Special Menu items are rendered as empty links in navigation
- 469c2a89 - Issue #3447837: Add tests for menu HTML markup
- 505d3959 - Issue #3447837: Rename class for consistency
- f45c496d - Issue #3447837: Rename variable to reduce confusion
- 987c15df - Issue #3447837: Address link use concern in toolbar-button.html.twig
- b1563219 - Issue #3447837: Move generated link tag logic out of the twig template, defer...
Toggle commit list-
ad8c8de9...a220b008 - 2 commits from branch
274 286 return $build; 275 287 } 276 288 289 /** 290 * Gets the tag type for the generated link of the given URL. 291 * 292 * @param \Drupal\Core\Url|string $url 293 * The URL object or string used for the link. 294 * 295 * @return string 296 * The HTML tag of the generated link of the given URL. 297 */ 298 public function getLinkTag($url) { 299 assert(is_string($url) || $url instanceof Url, '$url must be a string or object of type \Drupal\Core\Url'); 300 if (!$url instanceof Url) { 301 $url = Url::fromUri($url); -
As I understand it, avoiding link was talked about in the context of a SDC. In the discussion, we're assuming that
toolbar-button.html.twigwill become an SDC. Per @e0ipso, SDC's should be stateless and avoid Twig functions like link. By removing the call tolinkfromtoolbar-button.html.twigwe've achieved that. As such, I think using stateful Twig functions is OK, so long as their use is kept out of SDC, or here a template that is earmarked for becoming an SDC.Does that jive with your understanding as well @finnsky?
changed this line in version 19 of the diff
9 9 10 10 {% macro menu_items(items, attributes, menu_level) %} 11 11 {% for item in items %} 12 12 13 {% set item_id = ('navigation-link--' ~ item.original_link.pluginId)|clean_unique_id %} 13 14 {% if menu_level == 0 %} 14 15 {% if item.below is empty %} 15 16 <li id="{{ item_id }}" class="toolbar-block__list-item"> 16 17 {% include '@navigation/toolbar-button.html.twig' with { 17 attributes: create_attribute({ 'href': item.url|render, 'data-drupal-tooltip': item.title, 'data-drupal-tooltip-class': 'admin-toolbar__tooltip' }), 18 attributes: create_attribute({ 'href': item.url|render|default(null), 'data-drupal-tooltip': item.title, 'data-drupal-tooltip-class': 'admin-toolbar__tooltip' }), -
The addition here of
|default(null)will mean that whenitem.url|renderevaluates to empty string, which it does for non<a>elements, the overall value for thehrefattribute here will benull. When attributes are rendered, any null attributes are not rendered at all, so we won't have empty hrefs. I've tested this, and it looks to be the case. I believe this is resolved.Edited by m4olivei -
Thank you for the clarification @m4olivei! Missed that point.
changed this line in version 17 of the diff
9 9 10 10 {% macro menu_items(items, attributes, menu_level) %} 11 11 {% for item in items %} 12 12 13 {% set item_id = ('navigation-link--' ~ item.original_link.pluginId)|clean_unique_id %} 13 14 {% if menu_level == 0 %} 14 15 {% if item.below is empty %} 15 16 <li id="{{ item_id }}" class="toolbar-block__list-item"> 16 17 {% include '@navigation/toolbar-button.html.twig' with { 17 attributes: create_attribute({ 'href': item.url|render, 'data-drupal-tooltip': item.title, 'data-drupal-tooltip-class': 'admin-toolbar__tooltip' }), 18 attributes: create_attribute({ 'href': item.url|render|default(null), 'data-drupal-tooltip': item.title, 'data-drupal-tooltip-class': 'admin-toolbar__tooltip' }), 18 19 icon: item.class|clean_class, 19 html_tag: 'a', 20 html_tag: link_tag(item.url), changed this line in version 14 of the diff
87 94 $this->themeManager = $this->createMock('\Drupal\Core\Theme\ThemeManagerInterface'); 88 95 $this->dateFormatter = $this->createMock('\Drupal\Core\Datetime\DateFormatterInterface'); 89 96 $this->fileUrlGenerator = $this->createMock(FileUrlGeneratorInterface::class); 97 $this->linkGenerator = $this->createMock('Drupal\Core\Utility\LinkGeneratorInterface'); 90 98 91 $this->systemUnderTest = new TwigExtension($this->renderer, $this->urlGenerator, $this->themeManager, $this->dateFormatter, $this->fileUrlGenerator); 99 $this->systemUnderTest = new TwigExtension($this->renderer, $this->urlGenerator, $this->themeManager, $this->dateFormatter, $this->fileUrlGenerator, $this->linkGenerator); changed this line in version 19 of the diff
-
Thank you for working on this @m4olivei to unblock it.
Added some minor comments about the new approach.
The problem of this issue is not only that the expected tag was not being applied, but also any extra URL parameters like extra classes or
targetare lost.The core example pointed out, even if use that logic to determine the tag, ends up using
linkfunction to render the link to do not lose any attribute.Added extra test to confirm that menu item attributes are not lost
added 1 commit
added 56 commits
-
29806700...f4ae13d0 - 52 commits from branch
project:11.x - ca9bef17 - Issue #3447837: Attempt at a tests for link_tag twig function
- 982659e8 - Issue #3447837: Add surrounding quotes for data attribute
- f4817e86 - Merge branch '11.x' into 3447837-special-menu-items
- edccb71e - Issue #3447837: Avoid multiple calls to link_tag
Toggle commit list-
29806700...f4ae13d0 - 52 commits from branch
