Convert Olivero's tags to use single directory components

Created on 7 June 2023, over 1 year ago
Updated 15 September 2024, about 2 months ago

Olivero's tags component should be using SDC. Let's migrate it. The current code lives in core/themes/olivero/templates/field/field--node--field-tags.html.twig

User interface changes

Olivero out of box will show tags field label above.
But all that features from display view are available
- above
- inline
- hidden
- visually_hidden

๐Ÿ“Œ Task
Status

Needs work

Version

11.0 ๐Ÿ”ฅ

Component
Oliveroย  โ†’

Last updated about 1 hour ago

Created by

๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

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 @mherchel
  • First commit to issue fork.
  • last update over 1 year ago
    Custom Commands Failed
  • @stevenspriggs opened merge request.
  • Status changed to Needs review over 1 year ago
  • Converted Oliveros tags to single directory component

  • Status changed to Needs work over 1 year ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    CC failure in the MR.

    Tried testing though and after applying the MR and clearing cache I get this error

    Twig\Error\LoaderError: Template "olivero:tags" is not defined in "core/themes/olivero/templates/field/field--node--field-tags.html.twig" at line 42. in Twig\Loader\ChainLoader->getCacheKey() (line 99 of vendor/twig/twig/src/Loader/ChainLoader.php).
    
  • ๐Ÿ‡จ๐Ÿ‡ฆCanada Robert Ngo Montreal โšœ๏ธ

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

  • last update over 1 year ago
    Custom Commands Failed
  • @robert-ngo opened merge request.
  • last update over 1 year ago
    29,426 pass, 6 fail
  • Assigned to Robert Ngo
  • ๐Ÿ‡จ๐Ÿ‡ฆCanada Robert Ngo Montreal โšœ๏ธ
  • last update over 1 year ago
    29,427 pass, 5 fail
  • last update over 1 year ago
    29,427 pass, 5 fail
  • last update over 1 year ago
    29,427 pass, 5 fail
  • last update over 1 year ago
    29,427 pass, 5 fail
  • last update over 1 year ago
    29,427 pass, 5 fail
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

    After a quick review, this is looking really good!

    The only quick nit is that you can simplify your include function.

    instead of doing a

    {{ include('olivero:tags', {
      attributes: attributes,
      label_hidden: label_hidden,
    

    you can simplify to

    {{ include('olivero:tags', {
      attributes,
      label_hidden,
    

    Also, I'm guessing that the tests are failing because SDC is not being enabled by default. So tests won't pass at this point

  • last update over 1 year ago
    Custom Commands Failed
  • last update over 1 year ago
    29,427 pass, 5 fail
  • last update over 1 year ago
    29,404 pass, 7 fail
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States mherchel Gainesville, FL, US

    This is looking just about perfect! The only major change is to assemble the classes and title_classes arrays within the field--node--field-tags.html.twig file.

    Note that tests will not pass on this because SDC is not a part of standard profile. Once this gets to a good place we'll postpone it until SDC becomes stable.

  • last update over 1 year ago
    29,434 pass, 5 fail
  • Status changed to Needs review over 1 year ago
  • ๐Ÿ‡จ๐Ÿ‡ฆCanada Robert Ngo Montreal โšœ๏ธ
  • Status changed to Postponed over 1 year ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Changes look good (with my limited knowledge of SDC). I'm assuming the failures are due to SDC not being in standard. Postponing issue until that happens.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium dieterholvoet Brussels
  • Issue was unassigned.
  • Status changed to Active 4 months ago
  • ๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky

    Gonna work on it

  • Merge request !8860New tags approach - #3365389 โ†’ (Open) created by finnsky
  • Status changed to Needs review 4 months ago
  • ๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky

    I started a new approach because I believe that styles in components should not be tied to Drupal. This makes them more flexible.

    So it shouldn't be
    .field .node-- and other things

    Just pure simple and clear css

    Also fixed flexboxes and added support for label above

    Please check!

  • Pipeline finished with Success
    4 months ago
    Total: 475s
    #229838
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia ehsann_95

    The styles are looking as expected. Only thing i can figure out is the flex direction is column now. Attaching screenshot below

  • ๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky

    Yeah. I've added above/inline label display support.

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia ehsann_95

    @finnsky, This makes sense and the above/inline field display is working as expected.

    Inline:

    Above:

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    smustgrave โ†’ changed the visibility of the branch 3365389-convert-oliveros-tags to hidden.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    smustgrave โ†’ changed the visibility of the branch 3365389-sdc-olivero-tags to hidden.

  • Status changed to Needs work 4 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    If we are adding additional functionality believe the summary needs to be update to include this is more then just converting to SDC.

    Also this changes the current design with the label no longer being inline, which may need sub-maintainer sign off for that design change.

    Thanks.

  • Status changed to Needs review 4 months ago
  • ๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky

    more then just converting to SDC

    we should never just converting ;)

    SDC give us chance to do it better.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Normally 100% agree but would say in this case think getting to SDC first and in follow up doing enhancement would be best, just my opinion, or both seem to get delayed.

  • ๐Ÿ‡ซ๐Ÿ‡ทFrance pdureau Paris

    Hi,

    Looking at: https://git.drupalcode.org/project/drupal/-/merge_requests/8860/diffs

        items:
          title: Tags Items
          type: array
          item:
            type: object
            properties:
              content:
                title: Tags Item
                type: object
              attributes:
                title: Item Attributes
                type: Drupal\Core\Template\Attribute
    

    1. No need to have "Tags Items" as title, we know we are in a Tag component. "Items" is enough.

    2. This is not JSON schema valid. You need items instead of item:

          type: array
          items:
            type: object

    Source: https://json-schema.org/understanding-json-schema/reference/array

    3. In your JSON object, content property is an object itself. Why? An object of what? Which properties? It is printed in the template, but a mapping is not printable in Twig (except when it is a Drupal renderable, but those are expected in slots). Would a simple string fit instead?

    4. If you make content property a string, it would be the opportunity to adopt an existing Drupal data structure, like the menu structure, so it will be easier to plug it to Drupal API, by replacing content by title.

  • Status changed to Needs work 2 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Thanks @pdureau !

  • Pipeline finished with Success
    2 months ago
    Total: 537s
    #281131
  • Status changed to Needs review 2 months ago
  • ๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky

    Fixed feedbacks. Please review

  • Pipeline finished with Success
    2 months ago
    Total: 669s
    #281441
  • Status changed to Needs work about 2 months ago
  • ๐Ÿ‡ซ๐Ÿ‡ทFrance pdureau Paris

    Looks great.

    label prop

    I would recommend to make it a slot instead of a prop to be able to send any renderable into it. But I know slots are a bit underappreciated in SDC those days, and this is not a big deal anyway.

    tags prop

        tags:
          title: Tags
          type: array
          items:
            type: object
            properties:
              content:
                title: Tag
                type: string
    

    Only the first one is a blocker in my opinion.

    1. โš ๏ธ Now, we have an array of objects, where the object has only one property: content. Where did attributes go? Forgotten I guess, because visible in the template: <li{{ tag.attributes.addClass('tags__item') }}>{{ tag.content }}</li>

    2. As said previously, even without โœจ Add unified Links data type for menus, pagers, breadcrumbs... Active , you can make integration with Drupal API (menus, links, breadcrumbs..) easier by using text or title instead of content.

  • Pipeline finished with Failed
    20 days ago
    Total: 575s
    #317496
  • ๐Ÿ‡ท๐Ÿ‡ธSerbia finnsky

    Getting tonns of errors like

    [Tue Oct 22 20:50:22 2024] Uncaught PHP Exception Drupal\Core\Render\Component\Exception\InvalidComponentException: "[props.properties.tags.items.properties.attributes.type] Does not have a value in the enumeration ["array","boolean","integer","null","number","object","string"]/n[props.properties.tags.items.properties.attributes.type] String value found, but an array is required/n[props.properties.tags.items.properties.attributes.type] Failed to match at least one schema/n[props.properties.tags.items] Object value found, but an array is required/n[props.properties.tags.items] Failed to match at least one schema" at /Users/ivan/Sites/experiments/navi-core/core/lib/Drupal/Core/Theme/Component/ComponentValidator.php line 121
    [Tue Oct 22 20:50:22 2024] [::1]:49327 Closing

    when defining attributes in component. via object or that weird Drupal related type

Production build 0.71.5 2024