Migrate Toolbar button to SDC

I just checked and reviewed the latest MR, the changes resolved the Drupal\Core\Render\Component\Exception\InvalidComponentException: [extra_classes] String value found, but an array or an object is required in Drupal\Core\Theme\Component\ComponentValidator->validateProps() (line 203 of core/lib/Drupal/Core/Theme/Component/ComponentValidator.php) error.

Found couple of issues

https://gyazo.com/692ca676e368c55ff82a38f7591ec1c5
https://gyazo.com/8b102685e0872e13da8b733545be751b

Gonna check

I believe that SDC provides an excellent opportunity to clean up what was done quickly and sometimes roughly.
Therefore, I believe that in this task we should not do “the same thing only SDC”

We can rework and simplify this component. Remove quick and dubious decisions

I've started with different approach here.
First of all i cleaned all buttons. To make them more component friendly.

1. Moved safe triangle to own component
2. Cleaned CSS.
3. Transformed component props to make them more flexible.
4. Forced all elements on Navigation to use existing template.
5. Simplified cascades for collapsible variant
6. And after all that preparations i moved component to SDC.

Looks and works cool. Please review.

Fixed feedbacks. Thank you for review

Just a tiny thing that I noticed affecting some of the screen reader text.

Otherwise, changes look great to me! This is a really nice improvement by my read. Curious to see what the subsystem maintainers / folks with more familiarity with SDC will come back with. Nice work @finnsky

Gonna revert some css to fix bug with long text

https://gyazo.com/99dfdeeaf0ff3fcf82665da760e02b8a

Recent changes look good.

I came across one weird thing with this MR that I can't replicate on the 11.x branch. Here are some steps to reproduce it:

• ddev drush si -y standard
• ddev drush uli
• Login and enable navigation module via UI, eg. go to /admin/modules, enable Navigation module and Save

Expected behavior

Navigation looks and feels like its supposed to.

Actual result

If you clear cache, things do start working as expected. Not sure what is going on. Cannot reproduce on the 11.x branch, so seems like something here introduced it.

It is weird but seems that until cache clean SDC doesn't load toolbar-button css

core/modules/navigation/components/toolbar-button/toolbar-button.css

I reproduced issue locally and in tugboat instance

text prop

render filter on string?

text is a string prop, so why executing render filter on it? text|render|trim

To be split?

• it has no type and no description in the definition
• the two slicing operations is looking weird setAttribute('data-index-text', text|first|lower).setAttribute('data-icon-text', text|render|trim|slice(0, 2)|join('')) .

Are you sure it is not 2 different props?

Bad markup when empty

Also, a condition or a default value is missing because when text prop is empty or missing, we get this markup:

<button class="toolbar-button" data-index-text="" data-icon-text="">

So, why not doing something like that instead:

{% if text and text|length > 1 %}
{% set attributes = attributes.setAttribute('data-index-text', text|first|lower).setAttribute('data-icon-text', text|slice(0, 2)|join('')) %}
{% endif %}
{{ attributes.addClass(classes) }}

Other, smaller, feedbacks

Missing titles

Some props have a description but not title. It is better to add titles, they provide valuable documentation and can be used by UI leveraging the components.

attributes prop

This prop is automatically added to every component, so why doing manual declaration here:

    attributes:
      type: Drupal\Core\Template\Attribute
      title: Attributes
      description: Wrapper attributes.

Moreover, using a PHP namespace as a type is sometimes seen in the SDC community but still forbidden by JSON schema. So, let's avoid it when we can.

Fixed @pdureau feedbacks.
Keeping in NW status to check caching issue from #27

I've found interesting thing here.

When module enabled and cache not cleaned yet.
toolbar-button.css not loaded only in context of claro theme. when i go on homepage with standart install profile (olivero).
Toolbar button works as expected. When move back to claro css not loaded again

illustrate ^

Imo this is obvious not this issue false.
I think we can send it to review and open SDC ticket about it.

@finnsky I agree, there is pretty clearly some caching issue with SDC module components at play. I've filed the following issue: #3460598: Single directory component CSS asset library not picked up in admin theme immediately after module install without cache clear. It includes a sandbox module that I threw together as a minimal working example of the issue: https://git.drupalcode.org/sandbox/m4olivei-3460588

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

rebased

This is looking good to me. All of my feedback has been addressed as well as the points from @pdureau in #29 from my POV. Items and notes for that are below just to give full details. Marking as RTBC for me!

render filter on string?
text is a string prop, so why executing render filter on it? text|render|trim

✅ This has been fixed.

To be split?
it has no type and no description in the definition
the two slicing operations is looking weird setAttribute('data-index-text', text|first|lower).setAttribute('data-icon-text', text|render|trim|slice(0, 2)|join('')) .
Are you sure it is not 2 different props?

✅ Leaving this as one prop is the right approach, IMO. What we're doing here is deriving a two-letter acronym for use when a menu item doesn't have an assigned icon. The requirement is to always use the first two letters. The API for the component is simpler to just ask for the one prop, rather than force the consumer to pass both and do the work before passing in. We can also enforce a two-letter acronym this way.

Bad markup when empty
Also, a condition or a default value is missing because when text prop is empty or missing, we get this markup:

<button class="toolbar-button" data-index-text="" data-icon-text="">

✅ This has been fixed.

Missing titles
Some props have a description but not title. It is better to add titles, they provide valuable documentation and can be used by UI leveraging the components.

✅ This has been fixed.

attributes prop
This prop is automatically added to every component, so why doing manual declaration here:

✅ This has been fixed.

Leaving this as one prop is the right approach, IMO. What we're doing here is deriving a two-letter acronym for use when a menu item doesn't have an assigned icon. The requirement is to always use the first two letters. The API for the component is simpler to just ask for the one prop, rather than force the consumer to pass both and do the work before passing in. We can also enforce a two-letter acronym this way.

That's very interesting. Thanks.

Maybe it can be clearer in the Twig templates:

• by adding a comment
• and/or by creating intermediary variable(s) with explicit name(s)

Just an humble, non tested, proposal;

{% if text and text|length > 1 %}
  {% set icon_fallback = text|slice(0, 2)|join('') %}
  {% set attributes = attributes.setAttribute('data-index-text', text|first|lower).setAttribute('data-icon-text', icon_fallback) %}
{% endif %}

Thanks @pdureau, great suggestion. I've added a commend and variable for clarity.

few questions, as well as test failures

@nod_ it seems you reviewed wrong branch.

Thanks, just a dependency missing for safe triangle lib.

Also if your site uses navigation today it breaks it until you clear the cache. Should we have an empty update function to make sure it's done?

Fixed feedback.

Also if your site uses navigation today it breaks it until you clear the cache. Should we have an empty update function to make sure it's done?

we have ticket in SDC. I just set its prio to major.
https://www.drupal.org/project/drupal/issues/3460598

I don't know. Navigation is still experimental.

I'd like to get this in 10.3 so we need an update function to make sure cache gets cleared (per catch)

Added the empty update function to force a cache clear on update.

Merged the latest from the 11.x branch as #3460598: Single directory component CSS asset library not picked up in admin theme immediately after module install without cache clear was merged recently 🙌. In my testing locally, it doesn't look like the empty update function is needed anymore, but I'm not sure why that would be as #3460598 just added clearing the discovery cache on install.

if you're on 10.3 and already have the module installed you'd be in trouble without a cache clear, hence the empty update function

Ahh ok thanks @_nod for clarifying. I had not tested it installed on 10.3 and then upgrading to this branch. Makes sense then.

All ready for reviews! This will be a big one when it lands, b/c it opens the flood gates to getting the rest of the themeing from the navigation module switched to SDC.

Staying focused on the SDC part of the MR and considering the component as also an autonomous UI object which could be used outside of navigation module, here are some new feedbacks.

Modifiers, variants, classes & attributes

In the CSS, I see some modifier classes ("modifier" as the M in BEM)

• toolbar-button--primary
• toolbar-button--dark
• toolbar-button--large
• toolbar-button--small-offset
• toolbar-button--non-interactive
• toolbar-button--icon

Those modifier can be some states (altered by JS after the component is rendered), andtoolbar-button--icon is added by the icon prop, but others can be variants of the component (chosen when the component is rendered).

How the user would be able to control those variants? There is no dedicated prop and using the extra_classes prop is not friendly and explicit enough. Maybe an enumeration prop would be better (let's call it "variant" to be ready if #3390712: Add component variants to SDC lands in Core one day)

variant:
  type: string
  title: Variant
  enum:
   - primary
   - dark
   - large
   ...

With this snippet in template: {% set attributes = variant ? attributes.addclass('toolbar-button--' ~ variant) %}

It will depend of the "model" of this prop and how the values can be combined. We need to do a matrix with all possible combination.
For example, it mat be 2 different lists instead of one:

variant:
  type: string
  title: Variant
  enum:
   - primary
   - dark
   - ...
size:
  type: string
  title: Variant
  enum:
   - large
   - small-offset
   - ...

With this snippet in template:

{% set attributes = variant ? attributes.addclass('toolbar-button--' ~ variant) %}
{% set attributes = size ? attributes.addclass('toolbar-button--' ~ size) %}

Or something else...

By the way, if the only purpose of extra_classes was to add those variants, let's remove it. We also have the automatically added attribute prop to freely add classes, making such a prop unnecessary.

icon prop

It is a simple string:

    icon:
      title: Button icon
      type: string

How can we help the user to know which are the expected values? If it is an open list, we can add a description mentioning a documentation somewhere. If it is a controlled list, for example the files available in ./assets/ folder, we can add an enum:

    icon:
      title: Button icon
      type: string
     enum:
       - announcement
       - appearance
       - arrow-left
       - ...

text prop

The comment added in template is very valuable for developers:

  {# We take the first two letters of the button text to use as a fallback when
  the toolbar button does not have a pre-assigned icon. #} 

But the prop is still confusing for users. It is called "text" but it is never used as a proper text nor printed.

Can we add some information in the description ? And/or rename the prop?

Component & prop labels

Not very important, but if we do some new changes, let's also improve that:

• Component label: It seems Drupal community tends to agree on sentence case, and the prop labels are sentence case, but "Toolbar Button" is title case.
• Prop labels: No need to prefix them with "Button". The full component is a button, we know that already.

Twig linting

To be ready is #3284817: Adopt vincentlanglet/twig-cs-fixer for Twig coding standards lands in Core, and because it is a common practice in Twig community, we can add spaces between the print nodes brackets and the Twig tag:

-<{{html_tag|default('button')}} {{ attributes.addClass(classes) }}>
+<{{ html_tag|default('button') }} {{ attributes.addClass(classes) }}>
... 
-</{{html_tag|default('button')}}>
+</{{ html_tag|default('button') }}>

Rendering

Some work in progress tests. I will update this comment tomorrow.

By the way, if the only purpose of extra_classes was to add those variants, let's remove it. We also have the automatically added attribute prop to freely add classes, making such a prop unnecessary.

Not only in fact. In some cases we using concept of Bem Mix here
https://en.bem.info/methodology/css/#mixes

      extra_classes: [
        'admin-toolbar-control-bar__burger',
        'toolbar-button--small-offset',
      ],

And `admin-toolbar-control-bar__burger'` here is exactly extra class and we don't care about it in component.

For me, Variants are
- something that stores several modifiers
- something that cannot be combined with each other. That is, one button cannot be several variants at once

In our case, we have modifiers and not variants.

I added an array of modifiers and fixed them with an enum

I also left the extra_classes array so that you can add only those classes that are needed in the placement context, see ^ #52

Removed several modifiers that were not yet used and structured the icons. Fixed all the small reviews.

something that stores several modifiers

Some modifiers are variants (set at render time, server side), some are states (set after the rendering, browser side) or something else.

Something that cannot be combined with each other. That is, one button cannot be several variants at once

Indeed, that's an important characteristics of variants.

some are states (set after the rendering, browser side)

Actually no. All current modifiers are simple css classes added on server side.
For JS selectors we using data-attributes EG: [aria-controls="admin-toolbar"]

I agree that these data attributes can be structured somehow but i have no idea yet.

Can I get a confirmation that this would solve #3447837: Special Menu items are rendered as empty links in navigation as well?

Saw this posted in a slack channel and went to review but see that it's causing some test failures

 Drupal\Tests\navigation\Kernel\NavigationMenuMarkupTest::testToolbarButtonAttributes
    Drupal\Core\Render\Component\Exception\InvalidComponentException: [icon]
    Does not have a value in the enumeration
    ["announcements-feed-announcement","back","burger","cross","entity-user-collection","help","navigation-blocks","navigation-content","navigation-create","navigation-files","navigation-media","pencil","preview","shortcuts","system-admin-config","system-admin-reports","system-admin-structure","system-modules-list","system-themes-page","user"]

#57 I can't answer that but can try and keep an eye out while reviewing though.

@nod_

No. Special menu items is separated issue. We ignore it in this task for now.

In my opinion naming files in this way is ugly.
core/modules/navigation/components/toolbar-button/toolbar-button.css

Why not do this instead?

core/modules/navigation/components/toolbar-button/style.css

@trackleft2 thank you for feedback.
we follow SDC default assets naming

https://www.drupal.org/docs/develop/theming-drupal/using-single-director...

Good enough for me @finnsky, Thanks.

Thank you for you help!

There is still some Nightwatch error. But seems random. Sending to review.

Seems not random. We need to check what is going on on nightwatch

Yeah about to convey same these are not random failures, 'll give it try.

This is first time I have worked with nightwatch tests, tried to figure out root cause of issue & fix nightwatch test & left comment on MR.

Please review moving NR.

Rebased but work still needed. some unexpected test failures

Seems really random failures. Let's review it

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.

Hello Ivan,

modifiers prop

Following our discussion, you added this welcomed prop, but why repeating toolbar-button-- in every value:

    modifiers:
      type: array
      title: Modifier classes.
      description: Button modifications. These are defined in the component styles.
      items:
        type: string
        enum:
          - toolbar-button--collapsible
          - toolbar-button--dark
          - toolbar-button--expand--down
          - toolbar-button--expand--side
          - toolbar-button--large
          - toolbar-button--non-interactive
          - toolbar-button--small-offset
          - toolbar-button--weight--400

With a straightforward process in the template:

{% if modifiers is iterable %}
  {% set classes = classes|merge(modifiers) %}
{% endif %}

Doing that, you expose the complexity to the users instead of keeping it inside the template, in a safe place under your control. It also exposes the HTML class naming, which is an implementation detail, which may change later without any side effects.

The config and/or content entities will store the full string. The developers will need to type the full string. But they don't care. They want to manipulate directly collapsible, dark, large...

So why not doing that:

      ...
      items:
        type: string
        enum:
          - collapsible
          - dark
          - expand--down
          - expand--side
          - large
          - non-interactive
          - small-offset
          - weight--400

With:

{% set classes = classes|merge(modifiers|map(modifier => "toolbar-button--#{modifier}")) %}

Other feedbacks:

• Are you sure every modifier can be combined with every other modifier? Because your model makes it possible and this need to be checked.
• It seems you are using BEM Two Dashes style, with name--value notation for expand--down and expand--side. Do that mean it can be its own expand prop?
• You are also using this notation for weight--400 but there is only one value. Do you expect more later?

extra_classes prop

Still not fond of this one, but I will not die on this hill, so why not...

action prop

What "Expand or Collapse" means here? Is it the expected values? The usage context?

    action:
      type: string
      title: Action
      description: Hidden button action text. Expand or Collapse.

but why repeating toolbar-button-- in every value:

i don't thin we should really care about repeating here. but ok.

Are you sure every modifier can be combined with every other modifier? Because your model makes it possible and this need to be checked.

yes. they are designed so.

expand--down and expand--side. Do that mean it can be its own expand prop?

it is just css selector at the moment. probably later.

weight--400 but there is only one value. Do you expect more later?

yes. for now we have various buttons only in navigation and one in top bar.
https://gyazo.com/cdaff6debbde93c95747684b60e4a8a5
but should be much more later

Please review.

Looks OK to me now.

I have evaluated and tested the SDC component (YAML definition + Twig template) of this MR, without considering the big picture for the navigation module.

Once Twig 3.11 will land as a Drupal core dependency, the is iterable test will need to be replaced, but I will create a dedicated ticket for this task.

This patch makes installing menu_test through the UI crash the site. It's not a production issue but it is an indication that the fix in kernel tests is not sufficient.

On the code side I reviewed and OK with it, just need this menu_test issue resolved.

We want to make an enum for icons but we all know that in the end it will be an icon from the backend.
I see this as a contradiction and this contradiction is not worth continuing to block this important task.

That's why I removed the enum from icons. (If you want to return it, you need to rewrite the tests)

I also added detach.

Please review

Committed and pushed 390f87558be to 11.x and bddfccb8792 to 11.0.x and e7f6c9b5bcf to 10.4.x and 51a4a5fda1f to 10.3.x. Thanks!

😮 Fascinating to see this happen! Following the meta now — very interesting to see the move to SDC is considered a stable blocker!

Move to SDC became blocker because in current task we reworked lot of things from first versions, which sometimes were done quickly