Migrate Toolbar button to SDC

Created on 1 July 2024, 12 days ago
Updated 10 July 2024, 3 days ago

Problem/Motivation

SDC is part of core now, so we can migrate the existing code to take advantage of SDC.

Create Toolbar button into a SDC

Move the component Toolbar button into its own SDC.

Remaining tasks

User interface changes

None

API changes

Data model changes

๐Ÿ“Œ Task
Status

Needs review

Version

11.0 ๐Ÿ”ฅ

Component
Navigationย  โ†’

Last updated about 10 hours ago

No maintainer
Created by

๐Ÿ‡ช๐Ÿ‡ธSpain ckrina Barcelona

Live updates comments and jobs are added and updated live.
  • Needs subsystem maintainer review

    It is used to alert the maintainer(s) of a particular core subsystem that an issue significantly impacts their subsystem, and their signoff is needed (see the governance policy draft for more information). Also, if you use this tag, make sure the issue component is set to the correct subsystem. If an issue significantly impacts more than one subsystem, use needs framework manager review instead.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @ckrina
  • ๐Ÿ‡ช๐Ÿ‡ธSpain ckrina Barcelona
  • ๐Ÿ‡ช๐Ÿ‡ธSpain ckrina Barcelona
  • ๐Ÿ‡ช๐Ÿ‡ธSpain ckrina Barcelona

    ckrina โ†’ changed the visibility of the branch 3458215-migrate-toolbar-button to hidden.

  • ๐Ÿ‡ช๐Ÿ‡ธSpain ckrina Barcelona

    ckrina โ†’ changed the visibility of the branch 3458215-migrate-toolbar-button to active.

  • Pipeline finished with Failed
    12 days ago
    Total: 164s
    #212735
  • ๐Ÿ‡ช๐Ÿ‡ธSpain ckrina Barcelona

    ckrina โ†’ changed the visibility of the branch 3458215-sdc-button-component to hidden.

  • ๐Ÿ‡ช๐Ÿ‡ธSpain ckrina Barcelona
  • ๐Ÿ‡ช๐Ÿ‡ธ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.

  • Pipeline finished with Failed
    12 days ago
    Total: 512s
    #212758
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia Gauravvv Delhi, India

    Gauravvvv โ†’ made their first commit to this issueโ€™s fork.

  • Pipeline finished with Failed
    11 days ago
    Total: 551s
    #213469
  • ๐Ÿ‡ฎ๐Ÿ‡ณ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

    finnsky โ†’ changed the visibility of the branch 3458215-migrate-toolbar-button to hidden.

  • Pipeline finished with Failed
    9 days ago
    Total: 532s
    #215407
  • ๐Ÿ‡ท๐Ÿ‡ธ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

  • Merge request !8657Migrate Toolbar button to SDC - #3458215 โ†’ (Open) created by finnsky
  • Pipeline finished with Failed
    9 days ago
    Total: 164s
    #215454
  • Pipeline finished with Failed
    9 days ago
    Total: 164s
    #215466
  • Pipeline finished with Success
    9 days ago
    Total: 517s
    #215514
  • Pipeline finished with Failed
    9 days ago
    Total: 188s
    #215544
  • Pipeline finished with Failed
    9 days ago
    Total: 563s
    #215609
  • Pipeline finished with Success
    9 days ago
    Total: 615s
    #215623
  • Pipeline finished with Failed
    9 days ago
    Total: 164s
    #215750
  • Pipeline finished with Failed
    9 days ago
    Total: 162s
    #215780
  • Pipeline finished with Success
    9 days ago
    #215801
  • Pipeline finished with Failed
    9 days ago
    Total: 166s
    #215858
  • Status changed to Needs review 9 days ago
  • ๐Ÿ‡ท๐Ÿ‡ธ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.

  • Pipeline finished with Failed
    9 days ago
    Total: 167s
    #215865
  • Pipeline finished with Success
    9 days ago
    Total: 884s
    #215890
  • Status changed to Needs work 8 days ago
  • ๐Ÿ‡จ๐Ÿ‡ฆCanada m4olivei Grimsby, ON
  • Pipeline finished with Canceled
    8 days ago
    Total: 25s
    #216158
  • Status changed to Needs review 8 days ago
  • ๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky

    Fixed feedbacks. Thank you for review

  • Pipeline finished with Success
    8 days ago
    Total: 773s
    #216159
  • ๐Ÿ‡ช๐Ÿ‡ธ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 :)

  • Pipeline finished with Failed
    8 days ago
    Total: 708s
    #216792
  • Status changed to Needs work 7 days ago
  • ๐Ÿ‡จ๐Ÿ‡ฆ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

  • Pipeline finished with Success
    6 days ago
    Total: 574s
    #218004
  • ๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky

    Gonna revert some css to fix bug with long text

    https://gyazo.com/99dfdeeaf0ff3fcf82665da760e02b8a

  • Pipeline finished with Success
    6 days ago
    Total: 475s
    #218008
  • Status changed to Needs review 6 days ago
  • ๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky
  • Pipeline finished with Success
    6 days ago
    Total: 481s
    #218014
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia Meeni_Dhobale

    Meeni_Dhobale โ†’ changed the visibility of the branch 3458215-migrate-toolbar-button-sdc to hidden.

  • Status changed to Needs work 4 days ago
  • ๐Ÿ‡จ๐Ÿ‡ฆ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.

  • Pipeline finished with Success
    4 days ago
    Total: 601s
    #219256
  • ๐Ÿ‡ท๐Ÿ‡ธ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.

  • Pipeline finished with Success
    3 days ago
    Total: 574s
    #220351
  • ๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky

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

  • Pipeline finished with Success
    3 days ago
    Total: 469s
    #220359
  • ๐Ÿ‡ท๐Ÿ‡ธ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

  • ๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky

    illustrate ^

  • Status changed to Needs review 3 days ago
  • ๐Ÿ‡ท๐Ÿ‡ธ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

Production build 0.69.0 2024