Migrate Toolbar button to SDC

ckrina → changed the visibility of the branch 3458215-migrate-toolbar-button to hidden.

ckrina → changed the visibility of the branch 3458215-migrate-toolbar-button to active.

ckrina → closed merge request !8597

ckrina → changed the visibility of the branch 3458215-sdc-button-component to hidden.

ckrina → credited e0ipso → .

I just pushed the work that @e0ipso helped me do for the Toolbar button component during DrupalCon Portland. It's just initial work so feel free to pick it and follow the work from there.

Gauravvvv → made their first commit to this issue’s fork.

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

finnsky → changed the visibility of the branch 3458215-migrate-toolbar-button to hidden.

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

Adding "Needs subsystem maintainer review" to be sure we get a +1 from a SDC subsystem maintainer. This is a very important first step to integrate SDC into the admin UI and we better get it right :)

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

Meeni_Dhobale → changed the visibility of the branch 3458215-migrate-toolbar-button-sdc to hidden.

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: 🐛 Single directory component CSS asset library not picked up in admin theme immediately after module install without cache clear Active . 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

@nod_ it seems you reviewed wrong branch.

nod_ → closed merge request !8599

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 🐛 Single directory component CSS asset library not picked up in admin theme immediately after module install without cache clear Active

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 🐛 Single directory component CSS asset library not picked up in admin theme immediately after module install without cache clear Active 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 (controlled by JS after the component is rendered), andtoolbar-button--icon is added by the icon prop, but other 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 ✨ Add component variants to SDC Needs work 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) %}

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, 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.

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/key-concepts/#mix

      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 🐛 Special Menu items are rendered as empty links in navigation RTBC 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

Not much work with NIghtwatch test can give it try, it 'll be great if you can share hints/suggestions.