SDC components: Fix JSON schema usage

Created on 8 January 2024, 11 months ago
Updated 21 April 2024, 7 months ago

Problem/Motivation

Hi,

We are building UI Patterns 2.x, based on SDC API, and we are using Radix 6.x for testing our site building and contribution features (usage of SDC components in blocks, views, layouts, field formatters...).

Doing those tests, we have found some issues related to JSON schema usage.

Empty undefined object

This is not enough to be understood and processed: {"type":"object"}

Found in:

  • heading component, title_link prop. Must be a string, because printed like that <a href="{{ title_link }}"> in template
  • nav-item component, link prop. Must be a slot, because it expect a renderable array in template: <a href="{{ link['#url'] }}" class="nav-link {{ is_active ? 'active' }}">{{ link['#title'] }}</a>. However, I am not sure it is easy to use |add_class() filter to a link render element.
  • accordion component, title_link prop. Like heading's title_link because. Beware of prop drilling.

Array of undefined values

This is not enough to be understood and processed: {"type":"array"}

Slots?

title_prefix & title_suffix props in field-comment, page-title, comment, node components...
Are they slots? They looks like that in templates.

Array of HTML classes

classes, heading_utility_classes, body_classes, body_utility_classes... in html, user, comment,, nav...

Proposal:

type: array
items:
  type: string

By the way, sometimes classes are array, sometimes they are strings (example: wrapper_classes prop in toast). Also, this prop must not be necessary if the classes are directly injected in the default attribute prop, without any processing in the template.

Table rows?

table component, table prop. This is an hard one 🌱 What do we do with table rows and table cells data structure? Active . There are may proposal possible, but none of them are simple. But anyway, it seems the props is not used in the template!

Bag of props

configuration prop in block component is a bag of props:

{
    "type": "object",
    "properties": {
        "label_display": {
            "type": "string",
            "title": "Label Display",
            "description": "The display settings for the label."
        },
        "provider": {
            "type": "string",
            "title": "Provider",
            "description": "The module or other provider that provided this block plugin."
        }
    }
}

it would be more manageable to flat them into 2 props: label_display & provider.

A component hidden in a prop?

toasts prop in toasts component is very complex and too specific:

{
  "type": "array",
  "items": {
    "type": "object"
    "properties": {
      "header": {
        "properties": {
          "title": {
            "type": "string"
          },
          "subtitle": {
            "type": "string"
          },
          "classes": {
            "type": "string"
          }
        },
        "type": "object"
      },
      "body": {
        "type": "string"
      },
      "role": {
        "type": "string"
      },
      "with_close": {
        "type": "boolean"
      },
      "autohide": {
        "type": "boolean"
      },
      "delay": {
        "type": "integer"
      },
      "with_body_wrapper": {
        "type": "boolean"
      },
      "body_wrapper_classes": {
        "type": "string"
      },
      "attributes": {
        "type": "object"
      },
      "additional_buttons": {
        "items": {
          "properties": {
            "text": {
              "type": "string"
            },
            "color": {
              "type": "string"
            },
            "outline": {
              "type": "boolean"
            },
            "attributes": {
              "type": "object"
            }
          },
          "type": "object"
        },
        "type": "array"
      }
    }
  }
}

It seems a toast component is missing, with:

  • slots: header (?), body, additional_buttons(?)...
  • props: with_close, autohide, delay...

So, toasts can be a slot of toast components, and become more future-proof and manageable by tools like UI Patterns or SDC Display.

🐛 Bug report
Status

Fixed

Version

6.0

Component

Code

Created by

🇫🇷France pdureau Paris

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @pdureau
  • Hey Pierre, thanks for a thorough check on the components and sorry for a late reply, I've been quite busy but I'll start cleaning this up soon and keep you posted on that

  • Status changed to Needs review 9 months ago
  • Hi Pierre, so far I fixed/improved the following:

    • Heading component
    • Nav-item component
    • Accordion component
    • Table component
    • Array of HTML classes

    Regarding title_prefix & title_suffix, these variables are not slots in the sense used by Drupal's SDC schema, but they serve a similar purpose in terms of allowing customization of the page title, for Radix design philosophy, I treat twig blocks as slots and not any other variables

    I'd appreciate it if you check these changes out while I work on Toasts, thanks

  • Assigned to pdureau
  • 🇫🇷France pdureau Paris

    I will have a look

  • 🇫🇷France pdureau Paris

    Heading component

    https://git.drupalcode.org/project/radix/-/tree/6.0.x/components/heading

    Unused attributes variable

    heading_attributes is used instead of the default attributes variable.

    It is defined in the YML (like badge component do for badge_attributes, or page-footer do for footer_attributes) but it would be better to use the default attributes variable.

    And it will prevent this kind of verbose precautions:
    {% set heading_attributes = heading_attributes ?: create_attribute() %}

    Duality slots / props

    So, now we have heading_content slot as a Twig block AND title_link & content props inside the exact same block:

        {% block heading_content %}
          {% if title_link %}
            <a href="{{ title_link }}">{{ content }}</a>
          {% else %}
            {{ content }}
          {% endif %}
        {% endblock %}

    What will happen if we call the component with only the slot ?

    "#type" => "component",
    "#component"  =>"radix:heading",
    "#slots" => [
      "content" => ["#plain_text" => "Lorem ipusm"],
    ]

    Only the props ?

    "#type" => "component",
    "#component"  =>"radix:heading",
    "#props" => [
      "title_link" => "Foo",
      "content" => "Bar",
    ]

    Both slot and props ?

    Let's be careful with the Twig block system. It is useful for template inheritance, but in component driven development we don't do template inheritance.

    nav-item component

    https://git.drupalcode.org/project/radix/-/tree/6.0.x/components/nav-item

    Unused attributes variable

    nav_item_attributes is used in template but not defined anywhere.
    But as seen for heading component, instead of adding the prop in the definition, it is better to use the default attributes variable.

    link slot

    The template looks now OK:
    {{ link|add_class('nav-link', is_active ? 'active' : '') }}

    But not the definition yet. It went from undefined object to undefined array.

    Accordion component

    Not reviewed yet. I will update this comment later.

    Table component

    table prop is now split in 5 slots (table_caption, table_colgroup, table_header, table_body & table_footer) and 5 props:

    • caption: String
    • colgroups: Undefined array
    • header: Undefined array
    • rows: Undefined array
    • footer: String

    Like for heading component, what to do with duality slots/props ?

    Array of HTML classes

    Perfect :)

    title_prefix & title_suffix

    Indeed, they are not always slots. Fro example, {{ title_suffix.contextual_links }} in media component.
    But they need to be defined anywhere. What is expected inside ?

  • Issue was unassigned.
  • Status changed to Needs work 9 months ago
  • 🇫🇷France pdureau Paris

    Review done. I would be happy to continue this conversation if you still need me.

  • Pierre great work as always, by any chance would you like to be a co-maintainer? makes rather more sense that way and you can just start contributing if you have free time in between all your other projects

  • 🇫🇷France pdureau Paris

    Hi Sohail,

    I would be honoured to become maintainer of Radix, but I am already maintaining a Bootstrap5 theme: https://www.drupal.org/project/ui_suite_bootstrap

    Sharing feedbacks about SDC integration is easy for me, because it is very operational and, most of the time, simple binary decision (it is OK ? is it not?). However, becoming more involved and part of the team, I may feel uncomfortable dealing with such similar projects.

  • yeah I see your point, and I need to give your theme a go, thanks a lot anyways but feel free whenever you wanted to jump in, let me know.
    Otherwise I appreciate your contribution in general

  • 🇫🇷France pdureau Paris

    and I need to give your theme a go

    Great :)

    If you are interested about our SDC implementation (which will become default this summer I hope, replacing current UI Patterns 1.x components), we have a specific branch with them: https://git.drupalcode.org/issue/ui_suite_bootstrap-3412076/-/tree/34120...

    Except table component which is under rework, there are pretty good IMHO.

    links, variants and stories properties of components definitions are UI Patterns specific.

  • Status changed to Fixed 8 months ago
  • Thanks a lot for the help Pierre and a lot of good tips and hints, most are fixed now, some are opinionated but all is good so far. Feel free to re-open with your great suggestions, marking this as fixed for now

  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024