- Issue created by @ckrina
- Merge request !8597Draft: Issue #3442259 by catch, quietone, dww: Reduce time of Migrate Upgrade tests... → (Closed) created by ckrina
- Merge request !8599Draft: Issue #3458215: Initial work for the button component → (Closed) created by ckrina
- 🇪🇸Spain ckrina Barcelona
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.
- 🇮🇳India Gauravvv Delhi, India
Gauravvvv → made their first commit to this issue’s fork.
- 🇮🇳India Meeni_Dhobale
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. - 🇷🇸Serbia finnsky
Found couple of issues
https://gyazo.com/692ca676e368c55ff82a38f7591ec1c5
https://gyazo.com/8b102685e0872e13da8b733545be751bGonna check
- 🇷🇸Serbia finnsky
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
- Status changed to Needs review
2 months ago 2:36pm 4 July 2024 - 🇷🇸Serbia finnsky
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.
- Status changed to Needs work
2 months ago 8:38pm 4 July 2024 - Status changed to Needs review
2 months ago 9:29pm 4 July 2024 - 🇪🇸Spain ckrina Barcelona
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 :)
- Status changed to Needs work
2 months ago 7:36pm 5 July 2024 - 🇨🇦Canada m4olivei Grimsby, ON
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
- 🇷🇸Serbia finnsky
Gonna revert some css to fix bug with long text
- Status changed to Needs review
2 months ago 11:41am 7 July 2024 - 🇮🇳India Meeni_Dhobale
Meeni_Dhobale → changed the visibility of the branch 3458215-migrate-toolbar-button-sdc to hidden.
- Status changed to Needs work
2 months ago 9:13pm 8 July 2024 - 🇨🇦Canada m4olivei Grimsby, ON
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.
- 🇷🇸Serbia finnsky
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
- 🇫🇷France pdureau Paris
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.
- 🇷🇸Serbia finnsky
Fixed @pdureau feedbacks.
Keeping in NW status to check caching issue from #27 - 🇷🇸Serbia finnsky
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 - Status changed to Needs review
about 2 months ago 7:32am 10 July 2024 - 🇷🇸Serbia finnsky
Imo this is obvious not this issue false.
I think we can send it to review and open SDC ticket about it. - 🇨🇦Canada m4olivei Grimsby, ON
@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
- Status changed to Needs work
about 2 months ago 11:48am 19 July 2024 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.
- Status changed to Needs review
about 2 months ago 12:30pm 19 July 2024 - Status changed to RTBC
about 2 months ago 7:23pm 19 July 2024 - 🇨🇦Canada m4olivei Grimsby, ON
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.
- 🇫🇷France pdureau Paris
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 %}
- 🇨🇦Canada m4olivei Grimsby, ON
Thanks @pdureau, great suggestion. I've added a commend and variable for clarity.
- Status changed to Needs work
about 2 months ago 9:21pm 19 July 2024 - Status changed to RTBC
about 2 months ago 9:44pm 19 July 2024 - Status changed to Needs work
about 2 months ago 10:34pm 20 July 2024 - 🇫🇷France nod_ Lille
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?
- Status changed to Needs review
about 2 months ago 7:48am 21 July 2024 - 🇷🇸Serbia finnsky
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 ActiveI don't know. Navigation is still experimental.
- Status changed to Needs work
about 2 months ago 9:13pm 21 July 2024 - 🇫🇷France nod_ Lille
I'd like to get this in 10.3 so we need an update function to make sure cache gets cleared (per catch)
- Status changed to Needs review
about 2 months ago 8:45pm 24 July 2024 - 🇨🇦Canada m4olivei Grimsby, ON
Added the empty update function to force a cache clear on update.
- 🇨🇦Canada m4olivei Grimsby, ON
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.
- 🇫🇷France nod_ Lille
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
- 🇨🇦Canada m4olivei Grimsby, ON
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.
- Status changed to Needs work
about 1 month ago 5:10pm 31 July 2024 - 🇫🇷France pdureau Paris
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), and
toolbar-button--icon
is added by theicon
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
propIt 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
propThe 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.
- 🇷🇸Serbia finnsky
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/#mixextra_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.
- 🇷🇸Serbia finnsky
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 onceIn 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.
- Status changed to Needs review
22 days ago 9:00am 17 August 2024 - 🇫🇷France pdureau Paris
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.
- 🇷🇸Serbia finnsky
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.
- 🇫🇷France nod_ Lille
Can I get a confirmation that this would solve 🐛 Special Menu items are rendered as empty links in navigation RTBC as well?
- 🇺🇸United States smustgrave
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"]
- Status changed to Needs work
17 days ago 12:37pm 22 August 2024 - 🇺🇸United States smustgrave
#57 I can't answer that but can try and keep an eye out while reviewing though.
- 🇷🇸Serbia finnsky
@nod_
No. Special menu items is separated issue. We ignore it in this task for now.
- 🇺🇸United States trackleft2
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
- 🇷🇸Serbia finnsky
@trackleft2 thank you for feedback.
we follow SDC default assets naminghttps://www.drupal.org/docs/develop/theming-drupal/using-single-director... →
- First commit to issue fork.
- Status changed to Needs review
9 days ago 7:31am 30 August 2024 - 🇷🇸Serbia finnsky
Thank you for you help!
There is still some Nightwatch error. But seems random. Sending to review.
- Status changed to Needs work
9 days ago 7:40am 30 August 2024 - 🇷🇸Serbia finnsky
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.