Introduce component variants to SDC

Created on 29 September 2023, over 1 year ago

Problem/Motivation

We are planning to rewrite UI Patterns → upon SDC. To achieve this, we are proposing a few changes and additions:

Variants is a common feature in UI Components:

  • Bootstrap cards have vertical and horizontal variants
  • Material Design buttons have text, outilned, raised and unelevated variants
  • ...

Declaring variants as a prop is not enough for many reasons.

1. Using a string type with enum doesn't allow proper documentation (label, description):

props:
  type: object
  properties:
    variant:
      type: string
      enum:
        - primary
        - secondary
        - inverted

2. Using a anyOf with constant is verbose and complicated:

props:
  type: object
  properties:
    variant:
      anyOf:
        - { "const": "primary", "title": "Primary", "description": "..." }
        - { "const": "secondary", "title": "Secondary", "description": "..." }
        - { "const": "inverted", "title": "Inverted", "description": "..." }

3. The prop ID is free and it will not always be "variant", but sometimes "variants", "variations", "versions", "scheme"... SDC ecosystem may suffer of this lack of consistency. A module which want to leverage components variants will have no way of guessing which prop is a variant.

Proposed resolution

Add a new "variant" property at the root of component declaration with the same structure as the slot property :

name:  Card
variants:
  primary:
    title: Primary
    description: ...
  secondary:
    title: Secondary
    description: ...
  inverted:
    title: Inverted
    description: ...
  primary:
    title: Primary
    description: ...
props: {}
slots: {}

Proposal 1: declare as variant, use as prop

Once declared, the variant is loaded a prop and used as a prop:

[
  '#type' => 'component',
  '#component' => 'card',
  '#props' => [
    'variant' => 'primary',
  ]
]

This is what we plan to do in UI Patterns 2.x if it is not done at the SDC level, not because it is our preference, but because we don't want to "hack" SDC too deeply.

Proposal 1: declare as variant, use as variant

With the introduction of a new property in the render element:

[
  '#type' => 'component',
  '#component' => 'card',
  '#variant' => 'primary',
  '#props' => [ ]
]

We have a preference for this solution.

Remaining tasks

If there is a chance for this feature to be accepted, we can propose a merge request soon.

We have one month before the release of Drupal 10.2.0-alpha1.

API changes

Yes.

✨ Feature request
Status

Active

Version

10.1 ✨

Component
single-directory components  →

Last updated 1 day ago

Created by

🇫🇷France pdureau Paris

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

Merge Requests

Comments & Activities

  • Issue created by @pdureau
  • 🇫🇷France pdureau Paris
  • 🇫🇷France pdureau Paris
  • 🇫🇷France pdureau Paris
  • e0ipso Can Picafort

    Thanks for this! I am not sure weather or not we have an issue for this already, but I think this is something we need.

    For me, the main reasons to add variants is what you mention:

    This can be useful when the markup is changing a lot from a variant to another, and help keeping each Twig template clean and simple.

    This was once in the SDC codebase, and we stripped it away in order to reduce the scope for core inclusion. Perhaps this issue can help move this faster: #3317120: Remove variant support → .

  • 🇫🇷France pdureau Paris

    I have updated the issue summary to remove references to "distinct template by variant". We understand it can be a messy implementation for an uncommon use case, and we can get rid of this requirement.

  • 🇫🇷France pdureau Paris
  • e0ipso Can Picafort

    I am leaning towards proposal 2. This is how we have it on CL Components as well.

    I would also like to set expectations on the timeline for this. It think this needs to go in, after we move all the codebase into Drupal core itself.

  • 🇺🇸United States j. ayen green

    I like the idea of option 2. I'd like to add a use case here, as well. Having a component, such as an Event. There could be more than one display defined for the detail or teaser view, but both would be referring to the same component data so need a way to be supported by an appropriate variant.

  • e0ipso Can Picafort

    I believe BC will complicate things here, now that SDC is stable. But I am getting ahead of myself.

    I think we have to:

    Define how to call the variant from Twig

    This is the template ID. If we want to keep ourselves to native include / embed / extend we need to create a new naming convention. Potential ideas:

    include('<provider>:<component-name>#<variant>', ...)
    include('<provider>:<component-name>.<variant>', ...)
    include('<provider>:<component-name>--<variant>', ...)
    include('<provider>:<component-name>|<variant>', ...)
    

    Define the naming convention for the template file name

    Here we should probably mirror the decision above.

    <provider>/components/<component-name>/<component-name>--<variant>.twig
    <provider>/components/<component-name>/<component-name>.<variant>.twig
    <provider>/components/<component-name>/<component-name>|<variant>.twig
    

    Connect the template ID with the filename

    The ComponentLoader will receive a template ID, and will be in charge of finding the template file name, as defined above.

    For the render array way of embedding things, the ComponentElement will take a new optional #variant (as proposed in the IS), and will compose the necessary template ID. Everything else is the same as using Twig to embed the component.

    The component plugin Drupal\Core\Plugin\Component

    At this moment we access the template for the component via a public property $component->template. However now the template to be used for the component will depend on the variant. We need to introduce $component->getTemplate($variant) (which could throw a new MissingComponentVariantException). This is a problem for backwards compatibility.

    Proposals to tackle this issue should include a plan for backwards compatibility.

  • e0ipso Can Picafort

    This summary above is part of my conversations with @penyaskito on the topic. Please, consider granting credit to him.

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    I was working on this preceding Mateu's comment and reached that blocker.
    If we use the same component id per variant in plugin manager, we reach a point where the variant name is lost.
    Also, the template public prop is a show stopper for BC.

    Had another chat with Mateu today.

    An alternative sub-optimal implementation would be:

    - Generate a different component id per variant on plugin manager (as if they were derivatives).
    - Have some kind of "base component id" that we could use when we want to list components so they aren't repeated (API addition, should be BC)
    - Have method on plugin manager for listing components (excluding variants), and a separate one for the loading (with variants, would be the existing one). This should be BC, but would allow Storybook and like to list them without "duplicates".

    I think that would be still doable, but far from ideal. Otherwise this might be a won't fix.

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    I've pushed my WIP for availability, but that's the blocked road.

  • Pipeline finished with Failed
    12 months ago
    Total: 183s
    #179307
  • e0ipso Can Picafort

    I want to hear @pdureau's take on this. Let's see if I can lure him in to add his thoughts before we decide on anything.

  • 🇫🇷France pdureau Paris

    Some thoughts

    Define how to call the variant from Twig

    Like attributes, variant is a "glorified" prop:

    • both of them have a predefined and fixed prop type (Attribute object for attributes, string enum for variant)
    • attributes can be omitted from component's props definitions (because automatically added in templates), variant can be defined outside props definitions (in, order to easily add metadata on each enum value, without the need of using a complicated combination of anyOf with constant)

    So, because it is "just" a prop, why not injecting the value as a prop instead of inventing a new notation (like we already do for attributes)?

    {{ include('vendor:example', {
        variant: "primary",
        attributes: create_attribute().setAttribute(foo, "bar"),
        label: "Lorem ipsum",
        ...
      }, with_context = false) }}
    

    Define the naming convention for the template file name

    So, we are introducing such a feature? Since last summer, UI Suite → community is merging variants templates into components templates in order to be compatible with SDC. I believe many people will be happy if it is not necessary anymore.

    The 2 first proposals have my preference:

    <provider>/components/<component-name>/<component-name>--<variant>.twig because it looks like a template suggestion, so familiar.

    /components//..twig because:

    Current work

    If we use the same component id per variant in plugin manager, we reach a point where the variant name is lost.

    variant is a prop which is defined apart of other props because of the predefined type and the expected metadata. Why the component manager would have do complicated and/specific stuff about variants?

    In UI Patterns 2.x, we were hesitating between 2 solutions which look simpler:

    • duplicating the variants (defined apart from the other props in the YML) as a variant (string, enum) prop during the component definition loading in the component manager
    • or keeping them separated at the loading, and merging variant value into the props at rendering only

    Other feedbacks about variant definitions:

        "variantDefinition": {
          "type": "array",
          "patternProperties": {
            "^[a-zA-Z0-9_-]$": {
              "type": "string"
            }
          }
        },
    

    If variants are a simple list of variant ID, we are not bringing anything useful compared to simply using an enum prop.

    In an UI and design system point of view, variants have meaning. So, we need metadata to express and share this meaning:

        "variantsDefinition": {
          "type": "object",
          "patternProperties": {
            "^[a-zA-Z0-9_-]$": {
              "type": "object",
              "properties": {
                "title": {
                  "type": "string",
                  "title": "Title"
                },
                "description": {
                  "type": "string",
                  "title": "Description"
                }
              }
            }
          }
        }
  • Merge request !8197WIP separate components for the variants. → (Closed) created by penyaskito
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    > duplicating the variants (defined apart from the other props in the YML) as a variant (string, enum) prop during the component definition loading in the component manager

    A new approach for this at https://git.drupalcode.org/project/drupal/-/merge_requests/8197

  • Pipeline finished with Success
    12 months ago
    Total: 509s
    #183137
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Back to my original PR.

    Like attributes, variant is a "glorified" prop. [...] Why not injecting the value as a prop instead of inventing a new notation (like we already do for attributes)?

    The difference here is that attributes will be used on the template, while variant intends to modify the template being used.
    I gave another try at this, and unless we alter the twig EmbedNode object for ensuring it has that context (and then we need to modify the EmbedTokenParser class, definitely low level code), it's too late for that.

  • Pipeline finished with Failed
    12 months ago
    Total: 210s
    #183673
  • Pipeline finished with Failed
    12 months ago
    Total: 200s
    #183690
  • Pipeline finished with Failed
    12 months ago
    Total: 178s
    #183705
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    This needs sanity checks from @e0ipso.

  • Status changed to Needs review 12 months ago
  • 🇫🇷France nod_ Lille

    (I need this soon so I'm happy it's being worked on :) fixing status

    Regarding DX I would go with the MR 8152, while the overall code is (much) more complex, DX is nicer:

    {% embed 'umami:banner' with {
        variant: 'red',
        attributes: create_attribute(),
      } %}

    With the other solution, having the variant in the template name doesn't feel appropriate, it also exposes the separator which is another magic thing that non Drupal folks will need to learn.

    One question here would be what happens when you override a component in your theme but you don't have the specific variant ? In any case I'd avoid error-ing out and use the default template.

  • Status changed to Needs work 12 months ago
  • The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

  • 🇫🇷France pdureau Paris

    From @penyaskito:

    The difference here is that attributes will be used on the template, while variant intends to modify the template being used.

    You are adding the possibility of having a different template by variant, and some people will love this new feature.

    However, I hope:

    • this feature is optional (I don't have to create different template every time I create a new variant)
    • the variant prop will be injected into the template

    From experience, I believe most of the use of variants will be in class names, as BEM modifier or equivalents in other naming methodologies:

    {% set attributes = (variant and variant != 'default') ? attributes.addClass('foo-badge--' ~ variant) : attributes %}
    <p{{ attributes.addClass('foo-badge') }}>
    ...
    

    So, a single template is enough for all variants.

  • 🇫🇷France nod_ Lille

    Can we cut the separate template and see how much that simplifies the code for 8152?

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    @nod_ My main goal is having separate templates, and so far looks like that's the most complex part of it. I'm scared we might end up with a solution that prevents adding that later because of BC. Could we split this to a different issue for easiness of review and getting it in? Definitely. Should we do that before we have the templates covered? IMHO no.

  • 🇫🇷France nod_ Lille

    It's just that having a separate template for a variant is starting to scare me very much.

    If we make people associate "variant" with "separate html" I'm not sure we'll be able to close that door. And there are a few projects I've been on that I know would have abused that instead of making a proper solution, like some wild paragraph templating instead of a proper field formatter.

    I am not clear in which cases this is necessary vs. the real danger of duplicating html and making it really hard to keep things in sync over time. Can you provide a few use cases?

  • 🇫🇮Finland lauriii Finland

    Couldn't you load templates manually from the main template based on the variable? E.g. {% include "banner--" ~ variant ~ ".html.twig" %". With this, I'm not sure if we need to handle loading a variant template automatically. Otherwise we open a can of worms for example to decide what to do about CSS.

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    @nod_ We come from cl_components which allowed that, which might reinforce your argument that it's a door hard to close.
    I don't see how @lauriii's proposal couldn't work with the few cases where the template is significantly different and looks like the sanest thing at this point.

    Also @e0ipso already raised than having our own twig token parser in core is something he doesn't feel comfortable with. I would be happy to avoid writing that too :sweat

  • 🇫🇷France nod_ Lille

    If twig already allows it as lauriii suggested (and it's not even bad DX). Then we don't need to do anything special. Now we just need to not advertise it so that people won't do it :)

  • e0ipso Can Picafort

    From experience, I believe most of the use of variants will be in class names, as BEM modifier or equivalents in other naming methodologies:

    AFAICT you can accomplish that with a regular prop today, no need for this ticket to land.

    this feature is optional (I don't have to create different template every time I create a new variant)

    Couldn't you load templates manually from the main template based on the variable? E.g. {% include "banner--" ~ variant ~ ".html.twig" %".

    Just pointing out that this two statements may appear to be in conflict, but I believe they are not. We need to ensure the template file exists before doing the template swap.

    the variant prop will be injected into the template

    I think this may belong to the componentMetadata object that gets injected. Thoughts?

    I am still unconvinced about making the user request the variant using a seemingly regular prop. It feels like bad UX. I only see two inputs for embedding a component: the template name (my_module:my-component) and the template context (with {...}), so we don't have many options.

    If we are to introduce props with render logic, perhaps we could introduce an unlikely reserved prop for those. Then users would write:

    {% embed 'my_theme:cool-component' with {
      unlikelyReservedNameUpForBikeshedding: { variant: 'my-variant-name' },
      prop1: 'foo',
      prop2: 'bar',
    } %}
    

    In the future we could add more stuff in there (aside from variants), should we need to do so.

  • e0ipso Can Picafort

    At this point, it feels that this issue is only about a name convention. Variant is a regular prop: we can do that today. The template is managed manually: we can do that today.

    The only remaining value is to ensure that people uses variants with the same name, and thus it becomes introspectable metadata that we can use down the line (add a selector in XB?).

    Should we update the IS?

  • 🇩🇪Germany sharique

    Sorry, if I'm distracting from main topic, I want to highlight a scenario, which I think might be useful in making decision here, feel free to move it to separate ticket. So scenario is like this

    • There is a component in base theme called card.
    • For project A, created a new sub theme A
    • There is a requirement to a new variant of card in project A
    • Simple solution is to define a new variant in base theme, and implement it in sub-theme. Downside of this approach is in some cases it might break existing sites based base theme and base them may end up with many variant which are specific to projects.
    • Another approach is define a new variant in sub theme only. [It requires this ticket and 📌 Make SDC extensible Active ]. This will keep base theme clean.
  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

    Should we update the IS?

    That'd be very useful! 🙏

  • 🇺🇸United States xjm
  • 🇺🇸United States Kristen Pol Santa Cruz, CA, USA
  • 🇫🇷France pdureau Paris

    Hi @penyaskito,

    Following our chat at DrupalCon Barcelona, some examples of variants usages in UI Patterns 2 themes:

    Bootstrap 5

    variants:
      primary:
        title: Primary
      secondary:
        title: Secondary
      success:
        title: Success
      danger:
        title: Danger
      warning:
        title: Warning
      info:
        title: Info
      light:
        title: Light
      dark:
        title: Dark
    
    {% if variant and variant|lower != 'default' %}
      {% set attributes = attributes.addClass('alert-' ~ variant|lower|replace({'_': '-'})) %}
    {% endif %}
    

    https://git.drupalcode.org/issue/ui_suite_bootstrap-3412076/-/tree/34120...

    Daisy UI 4

    variants:
      default:
        title: Default
      info:
        title: Info
      success:
        title: Success
      warning:
        title: Warning
      error:
        title: Error
    
    {% if variant %}
    {% set attributes = (variant != 'default') ? attributes.addClass('alert-' ~ variant) : attributes %}
    {% endif %}
    + some stuff about icons
    

    https://git.drupalcode.org/project/ui_suite_daisyui/-/tree/4.0.x/compone...

    USWDS 3

    variants:
      info:
        title: Informative
      warning:
        title: Warning
      error:
        title: Error
      success:
        title: Success
      emergency:
        title: Emergency
    
    {% set role = 'alert' %}
    {% if variant and variant|lower != 'default' %}
      {% set attributes = attributes.addClass('usa-alert--' ~ variant) %}
      {% if variant|lower == 'success' %}
        {% set role = 'status' %}
      {% endif %}
    
      {% if variant|lower == 'info' or variant|lower == 'warning' %}
        {% set role = 'region' %}
      {% endif %}
    {% endif %}
    

    https://git.drupalcode.org/issue/ui_suite_uswds-3412077/-/tree/3412077-a...

  • 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    penyaskito → changed the visibility of the branch 3390712-add-component-variants to hidden.

  • I am not convinced about the necessity of implementing template suggestion for variants 🤔.

    I think it is enough to add the `variants` object at the root of the component declaration.

    IMHO it becomes confusing to mix the concepts of variations and display modes.

    A variant prop is enough for developers to create an infinity of variations (e.g. includes, embed... simply thanks to Twig):

    {# mysdc.twig #}
    <div{{ attributes.addClass(variant|clean_class) }}>
      {% if variant == "averydifferentvariationrequiringanothertemplate" %}
        {% include "mymodule/mysdc/mysdc--extrathingstodisplayforthisverydifferentvariation.twig" %}
      {% endif %}
      {{ title }}
      {{ body }}
      {{ cta }}
    </div>
    

    BTW: Can we clarify if variants if a single value or if it is an array?

  • 🇺🇸United States dalemoore

    I just saw this issue and wanted to mention: please don't rely that people will only be using Twig to render variant types. There are some of us who will be using Web components (done in e.g., Lit, FAST) for our SDCs and so will be making use of a variant prop to select the variant, and that is expecting a string value of the variant name (e.g., primary, secondary, etc.). The JS and constructed styles of the Web component will be the thing that determines how the variant will look, what slots each variant has, etc., the my-component.twig will only be to print stuff out into appropriate props/slots of the Web component. Similar to how I see in XB they're using the Shoelace components.

    Introducing something like this:

    {% embed 'my_theme:cool-component' with {
      '#variant': 'my-variant-name',
      prop1: 'foo',
      prop2: 'bar',
    } %}
    

    With the #variant prop seems to be introducing a Drupalism into a Web component that may be used elsewhere outside Drupal (that is, no other props will start with #).

    As long as I can do this instead:

    {% embed 'my_theme:cool-component' with {
      variant: 'my-variant-name',
      prop1: 'foo',
      prop2: 'bar',
    } %}
    

    By pulling the string value from #variant or any other method proposed I will be good though!

  • 🇫🇷France pdureau Paris

    Hi Dale,

    The proposal with #variant is for the render element only.

    Don't worry, the call from template will still be:

    {% embed 'my_theme:cool-component' with {
      variant: 'my-variant-name',
      prop1: 'foo',
      prop2: 'bar',
    } %}
    

    or:

    {{ include('my_theme:cool-component', {
      variant: 'my-variant-name',
      prop1: 'foo',
      prop2: 'bar',
    }) }}
    
  • Pipeline finished with Failed
    5 months ago
    Total: 163s
    #379192
  • Pipeline finished with Failed
    5 months ago
    Total: 149s
    #379215
  • Pipeline finished with Failed
    5 months ago
    Total: 110s
    #379218
  • Pipeline finished with Failed
    5 months ago
    Total: 99s
    #379224
  • Pipeline finished with Failed
    5 months ago
    Total: 135s
    #379229
  • Pipeline finished with Failed
    5 months ago
    Total: 120s
    #379237
  • Pipeline finished with Success
    5 months ago
    #379242
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Still missing:

    - Schema
    - Schema validation
    - Schema validation tests.

  • Pipeline finished with Canceled
    5 months ago
    Total: 1102s
    #379248
  • Pipeline finished with Failed
    5 months ago
    #379253
  • Pipeline finished with Success
    2 months ago
    Total: 514s
    #448445
  • Pipeline finished with Failed
    2 months ago
    Total: 629s
    #448552
  • Status changed to Needs review 2 months ago
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    In February I went through this with @e0ipso when we were at the same room.
    I've gone through his comments.
    This still needs improvements in validation (which I might need help with) + adding more tests, but feels like could have another review.

  • Pipeline finished with Success
    2 months ago
    Total: 400s
    #448561
  • 🇫🇷France pdureau Paris

    Thanks so much. I will have a look.

  • 🇫🇷France pdureau Paris

    1

    I am happily surprised to see you have added $element['#variant']. I believe our 2 "special" component props (attributes and variant) needs their own (optional) render properties and I need to create the related ticket for #attributes.

    2

    $component_attributes['data-component-variant'] = $context['variant'];
    

    That's surprising too but why not.

    3

      variantDefinitions:
        default:
          label: Default
          description: My default variant
    

    Why a complicated lower camel case property: variantDefinitions. I am already quite a few people struggling with libraryOverrides instead of the more straightforward and expected library. Can we have this instead:

      variants:
        default:
          label: Default
          description: My default variant
    

    4

    In my-cta.component.yml, variantDefinitions is inside the prop property:

    props:
      type: object
      required: []
      properties: {}
      variantDefinitions: {}

    It is not compliant with JSON schema standard. Can we move it to the component definition root?

    5

    We use label but:

    • component is using name
    • slots are using title
    • props are using title

    Instead of introducing a third way of declaring the same information, can we copy the current slot structure?

    6

    Is it not too late?

        if (empty($variant) || isset($context['variant'])) {
          $template .= sprintf('{%% embed \'%s\' %%}', $id);
        }
        else {
          $template .= sprintf('{%% embed \'%s\' with { variant: \'%s\'} %%}', $id, $variant);
        }
    

    Why tweaking the dynamically generate template instead of moving the variant to the props a bit earlier?

    Non tested naive code snippet:

        if (isset($element["#variant"] && other_condition_we_may_add)) {
          $element["#props"]['variant'] = $element["#variant"];
        }
    
  • Pipeline finished with Failed
    about 2 months ago
    Total: 531s
    #456900
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    3,4,5 fixed.

    6. It is not late.

    > Why tweaking the dynamically generate template instead of moving the variant to the props a bit earlier?

    Because with the current solution, if you already had a prop named variant because of our lack of support until now, we will be BC compatible ensuring not breaking your component (even if that's pretty unlike to break tbh). So you could have both "modern" components with variants defined and "oldest" components were you had a variant prop. Both will work, and you would have _some way_ to discern between each case.

    Also I kinda like having the variant explicitly in the generated template if someone ever gets to debug this deep.

    As far are I tested both solutions have the same result, but unless you have a really strong opinion on this I'd prefer having it explicitly.

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
  • 🇫🇷France pdureau Paris

    Because with the current solution, if you already had a prop named variant because of our lack of support until now, we will be BC compatible ensuring not breaking your component (even if that's pretty unlike to break tbh). So you could have both "modern" components with variants defined and "oldest" components were you had a variant prop. Both will work, and you would have _some way_ to discern between each case.

    Also I kinda like having the variant explicitly in the generated template if someone ever gets to debug this deep.

    I am not sure we will keep ComponentElement::generateComponentTemplate() forever.

    This method is doing complex logic, and dynamically generating a confusing "proxy" template, where loading directly the component template in '#type' => 'inline_template' would be enough.

    Of course, it was not done without a reason. It was added to make slots usable as Twig blocks, because:

    • template to template usage was heavily promoted at the beginning of SDC, as a strategy to "seduce" the current Drupal themers, working with node.html.twig, field.html.twig, block.html.twig...
    • there was this idea that Twig blocks would be more convenient for them than just printing the slots

    But we are moving away from those Drupal templates, the new display builders (like Experience Builder → or the UI Suite's Display Builder → ...) are skipping those templates and the service managing those templates, the ThemeManager, may be deprecated.

    So let's not add more stuff in this method.

  • Pipeline finished with Success
    about 1 month ago
    Total: 429s
    #463995
  • Pipeline finished with Failed
    about 1 month ago
    Total: 761s
    #464009
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    @pdureau You convinced me, no changes in ::generateComponentTemplate() and moved that logic to ::preRenderComponent as suggested. Thanks for taking the time to explain the reasoning behind doing that.

    Only pending thing is some test coverage @e0ipso requested, and verify if json schema validation is being triggered properly, as I don't think we need any extra validation that that?

  • Pipeline finished with Success
    about 1 month ago
    Total: 465s
    #464026
  • Pipeline finished with Failed
    about 1 month ago
    Total: 102s
    #464056
  • Pipeline finished with Success
    about 1 month ago
    Total: 1156s
    #464060
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    I've debugged the JsonSchema/Validator and still can't find why invalid data is accepted.
    This might need fresh eyes.

  • Pipeline finished with Failed
    about 1 month ago
    Total: 484s
    #465082
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Paired 1.5h with @larowlan and we got with the problem, fixed it and we added a test proving that validation is happening.

    The problem came from copy-pasting slots json schema definition. The problem is originally there and we all overlooked it :-(
    Created 🐛 SDC slots not being validated against json config schema Active for fixing this for slots.

    Thanks a lot Lee! (please credit him)

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
  • Pipeline finished with Success
    about 1 month ago
    Total: 567s
    #465099
  • Pipeline finished with Success
    about 1 month ago
    Total: 574s
    #465190
  • Pipeline finished with Failed
    about 1 month ago
    Total: 111s
    #465209
  • Pipeline finished with Success
    about 1 month ago
    Total: 695s
    #465215
  • 🇫🇷France steveoriol Grenoble 🇫🇷

    And how do you attach JS to a specific variant?
    Should a “{{ attach_library(‘mytheme/xxxxxx--variant’) }}” be added to the “xxxxxx--variant-teaser.twig” file, which is currently included in “xxxxxx.twig” with a condition on the variant, or is there a naming convention like “xxxxxx--variant-teaser.js” or “libraryOverrides” per variant in the xxxxxx.component.yml?

  • 🇫🇷France pdureau Paris
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    @steveoriol As with templates mentioned in #24, #27 and #28, there won't be anything specific for including different js files.

    You can still:

    a) Use the variant for having different classes in your twig, and your js can behave differently based on that.
    b) Use {{ attach_library('mytheme/mycomponent-variant-2') }} conditionally depending on the variant.

  • 🇫🇷France steveoriol Grenoble 🇫🇷

    OK, thank you penyaskito.

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Updated issue summary to reflect more clearly what we are doing here.

    I didn't want to delete such a great writing of alternatives from @pdureau, but moved to the bottom of the IS for clarity.

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
  • 🇺🇸United States phenaproxima Massachusetts

    I think this patch makes sense. I have a few minor questions but would otherwise gladly sign off on it (with the caveat that I am not, primarily, a front-end developer...but this level of SDC stuff does seem easy enough to grasp).

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Reading the code there's not much in the MR now which is a good sign.

    My immediate question coming from the code side first (Without reading here first) was isn't variant just a special prop? Why can't we just use a prop?
    Then I came here and read the issue summary and saw the reasoning

    Declaring variants as a prop is not enough for many reasons.

    - but then the question becomes what if a component needs more than one dimension in which to modify things?
    i.e. what if there is a need for two different 'variants' - the examples given only use one. How would we handle that?

    So playing devils advocate, this then leads to the question is the issue here that the schema for enum props too narrow in that it doesn't allow us to define human-readable names? Which of course leads to ✨ Enum vales do not have translatable labels Active . So that begs the question if we did ✨ Enum vales do not have translatable labels Active is the need for variant as pressing? Because with that we can have multiple dimensions on which to vary the component.

    Take the button component from the New South Wales (NSW) design system - https://designsystem.nsw.gov.au/components/button/index.html

    Do I make my variant the colour (dark, light, white, danger) or the style (filled, outline, outline solid). Where-as with the meta:enum proposal I just have one prop for each and they're both first-class citizens in terms of the dimensions I can chose.

    I realise 'variants' are an existing thing in design systems - e.g. MUI button - https://mui.com/material-ui/react-button/ it has text/outlined/countained - but then if we were representing that component in Drupal we'd be forced to use a prop for size and color and those are also enums. So with the meta:enum proposal those props are as important as the variant, but with this approach they're secondary considerations.

    Basically what I think is missing from the issue summary is why is variant treated as a special class of prop while other props might need the same affordances listed in the issue summary as considerations:

    1. Using a string type with enum doesn't allow proper documentation (label, description):
    2. Using a anyOf with constant is verbose and complicated:
    3. A module which want to leverage components variants will have no way of guessing which prop is a variant.

    All three of these reasons could apply to any enum prop.

  • 🇫🇷France nod_ Lille

    Variants are not the only concepts, we also have styles and themes/modes. Next step is ✨ Add a style utility API Active and XB will need something like https://www.drupal.org/project/ui_skins → sooner than expected too.

    So you're right that variant are not the only dimension, it's just that the other dimensions are not at the component level, they're a higher level concern.

  • 🇷🇸Serbia finnsky

    I can't say much about the code, but I'll speak conceptually.

    I like SDC for the same reason that I once loved BEM, namely:
    Simplicity and sufficient universality in describing everything.

    That is:
    BEM - Block + Element + Modifier
    SDC - Component + Slot + Prop

    Simplicity and universality are exactly what the standard needs. And if we continue the analogy with BEM, then we have

    BEiM - Block Element Modifier and Improved Modifier (Renderer)

    Which is not scary in general, but complicates the simplicity and elegance of the standard.

    I understand that many people want this and even understand their need, so in general I am not against it.
    But the only thing I would like to do is to leave these variants and other things outside the main SDC properties (props/slots)

    1. Components can work without mentioning Variants
    2. Variants are part of the standard, but they are not required to be used. The documentation may contain lines about the optionality of variants. For example, add Variants when you need a description of properties or a different render

    And so I like where we are going with these components! Thanks everyone!

  • Pipeline finished with Failed
    22 days ago
    #480125
  • Pipeline finished with Canceled
    22 days ago
    #480131
  • Pipeline finished with Success
    22 days ago
    #480138
  • 🇫🇷France Grimreaper France 🇫🇷

    Hi,

    About comment 61:

    Do I make my variant the colour (dark, light, white, danger) or the style (filled, outline, outline solid)

    The convention/best practice we adopted in UI Suite themes for this problematic is, "variants are defined based on the component classes variation, we separate multidimensional variant names with double underscore (a little bit like template suggestions)", this satisfies all (or almost all cases I have not checked before writing this comment) encountered in public design systems.

    Example with the button in UI Suite Bootstrap.

    Bootstrap's button can accumulate:
    - base class: btn
    - color class: btn-primary btn-secondary etc.
    - size class: btn-sm btn-lg

    YAML:

    variants:
      primary__sm:
        title: "Primary small"
      secondary__sm:
        title: "Secondary small"
    ...
      primary:
        title: Primary
      secondary:
        title: Secondary
    ...
      primary__lg:
        title: "Primary large"
      secondary__lg:
        title: "Secondary large"
    ...
    

    Twig:

    {% if variant and variant != 'default' %}
      {% set variants = variant|split('__')|map(v => v|replace({(v): 'btn-' ~ v})|replace({'_': '-'})) %}
      {% set attributes = attributes.addClass(variants) %}
      {% set attributes = attributes.addClass('btn') %}
    {% endif %}
    ...
    
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    I'll work on this this weekend, still hopeful on getting this in for 11.2.

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    Nothing really actionable here. NR so the discussions continue.

    @grimreaper #64: Wasn't aware of that, that's clever!

  • Pipeline finished with Canceled
    11 days ago
    Total: 544s
    #488443
  • Pipeline finished with Success
    11 days ago
    Total: 571s
    #488450
  • Pipeline finished with Failed
    11 days ago
    Total: 159s
    #488471
  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    All threads resolved, and added some test coverage we didn't have for twig debug info.
    After quite some reviews I hope this is RTBC material now.

  • Pipeline finished with Success
    10 days ago
    Total: 5182s
    #488486
  • 🇫🇷France pdureau Paris

    Hello all

    That's wonderful to see such a long awaited feature implemented with only 22 lines of PHP code (tests removed) despite 1 year of hard work.

    I consider the change ready. I would like to commit it myself. However, I will not be against some complementary validation because, even if I didn't participate to the code, I am was fully invested on this change.

    To answer @larowlan (@grimreaper and @finnsky already shared nice answers, but this is my take):

    So playing devils advocate, this then leads to the question is the issue here that the schema for enum props too narrow in that it doesn't allow us to define human-readable names?

    meta:enum is bringing us labels for enums. But we will also need other metadata (description for now, maybe more later) because the variants are sometimes heavily documented upstream and we need to be able to port this documentation).

    And it is not only about metadata. It is about having a (optional) prop which have the same name (variant) and the same type (enum of strings) for everybody.

    variant must be a glorified prop because:

    • it is a design concept, discussed with upstream (designers, business deciders...), shared with downstream (site/display builders...)
    • it can be (optionally) found in any component, which is not the case of other props which are more component specific

    Because with that we can have multiple dimensions on which to vary the component.

    It is up to the component author to decide what to do:

    • they can promote on of the enum as the variant enum. For example the one the most significant in a design point of view.
    • they can do multidimensional workaround with a delimiter as shared by @grimreaper
    • they can simply skip the variant and rely of a set of enums

    Most of the time, the solution will come from the upstream documentation or discussion with the design team. With this change, we are providing a convenient tool for this solution to be found.

  • 🇦🇺Australia alex.skrypnyk Melbourne

    I am with @larowlan on this.

    Why is this a special prop? Are you prescribing a certain workflow by providing this as a special prop?

    The variants (as in Design System UI Kit component variants or "variations") are no more than specific instances of components. The components do not know anything about "variants" - it is simply outside of the component's context.

    With introduction of `variant` and then referring to it as a variable for a conditional in the component you are:

    1. Locking to a single dimension

    2. Confusing the developers on what it is (yours is not a variant in a sense Design Systems use) and how to use it

    4. Introducing a workflow-specific (implementation-specific) way that is not a standard in developing Design Systems

  • 🇫🇷France nod_ Lille

    I don't understand what you're proposing.

    The implementation here does not prevent anyone from implementing variants as you describe in contrib. It's entirely optional so you can not use it and implement something else, or am I missing something?

    I can agree that variants could be a set of things defined outside of the sdc, but we don't have another intermediate level that can be used to describe what the variants are.

    Just to clarify the code smell you pointed at is not in the MR.

  • 🇦🇺Australia alex.skrypnyk Melbourne

    The proposal in this issue encourages placing non-production code directly into production component templates:
    https://git.drupalcode.org/project/ui_suite_bootstrap/-/blob/5.1.x/compo...

    I recommend against this approach and suggest exploring a cleaner alternative for defining variants.

    A better solution would be to declare variants in the component’s YAML schema as sets of prop values. That’s what I initially expected based on the issue title. The “don’t like it, don’t use it” argument doesn’t apply here — I do want to define and use variants, just not by embedding non-production logic in production templates.

    Instead of:

    name:  Card
    variants:
      primary:
        title: Primary
        description: ...
      secondary:
        title: Secondary
        description: ...
      inverted:
        title: Inverted
        description: ...
      primary:
        title: Primary
        description: ...
    props: {}
    slots: {}
    

    I propose this schema (similar to what Storybook does):

    name:  Card
    variants:
      primary:
        title: Primary
        description: ...
        props:
           kind: button
           text: my button
           third_prop: value13
    
      secondary:
        title: Secondary
        description: ...
        props:
           kind: link
           text: my button
           third_prop: value32
    props: {}
    slots: {}
    

    Here, variants.<name>.props would be validated against the declared props.*. Then any wrapper (e.g., for Storybook or style guide rendering) can consume and render the SDC component properly — without leaking non-production logic into production code.

  • 🇷🇸Serbia finnsky

    #71 Yeah!

    This really does look better!

    And if variants are just property bundles then they should work as bundles
    And slots and props remain basic and simple.

    This code really shouldn't be in components

    {% set variants = variant|split('__')|map(v => v|replace({(v): 'btn-' ~ v})|replace({'_': '-'})) %}
    
  • 🇫🇷France nod_ Lille

    Much clearer, thanks

  • 🇫🇷France nod_ Lille

    Opened ✨ Add component variants to SDC, storybook style Active to discuss the new proposal. It's a terrible issue title and issue summary but it exists :)

    As far as I'm concerned this issue is still RTBC, the approach is to declare variants the same way slots are declared, it's easy for people writing the sdc yaml to remember, title/description for metadata, variant name available inside the twig template. The MR implements that correctly.

    Now we're having some of the assumptions being questioned. Variants should be a predefined set of props, and variant name should not be accessible from the twig. There are many questions I have with the new proposal and to not derail this issue further I opened ✨ Add component variants to SDC, storybook style Active to discuss.

    If we think that the new approach is better we'll close this one and credit all the folks that participated. If the new approach doesn't work out, we'll go ahead with this one as-is.

  • 🇫🇷France nod_ Lille

    Or could this be a follow-up? since the new proposal is to add "props" to what we already have in this MR, feels like something additional to the current approach and not fundamentally incompatible? Having the variant "leak" into the twig template might not be ideal but it might still happens with the new approach for some Drupal reason.

  • 🇦🇺Australia alex.skrypnyk Melbourne

    @nod_
    Thank you for being understanding and proactive — I truly appreciate it.

    My concern with exposing variant data is that it risks becoming the de facto standard for SDC. This could unintentionally shape the official API and force all SDC implementations to adopt this approach.

    I get that there’s no Storybook-like wrapper yet, and this feels like a quick way to unblock UI Patterns (or similar projects, as noted in the issue description). But adopting this pattern prematurely raises questions about why certain contrib approaches should influence core direction over others.

    I recommend updating the MR to avoid "leaking" the variant data into template, but still allowing the schema to have the variant data available in the format specified in the description. The Storybook-like format can be handled later.
    The contrib implementations could start working on their own wrappers meanwhile and ship them as a part of their modules/themes.

    Thanks again.

  • 🇫🇷France pdureau Paris

    Hi Alex,

    Thanks for your proposal and for teaching me what is a "standard way of developing a design system".

    The proposal in this issue encourages placing non-production code directly into production component templates:

    I agree the syntax you are proposing is fixing the "code smell" shared by @grimreaper in #64 ✨ Introduce component variants to SDC Active , when a variant can be modelled a mix of enum instead of a single enum.

    But let's not forget it is a very specific and rare example, which could be fixed in other ways (by picking only one of those enums as the variant, for example).

    A quick and naive look on my local workspace shows it is found in less 2% of the 1000 components I have here:

    Do you have real life example of design systems heavily relying on this mechanism?

    2. Confusing the developers on what it is (yours is not a variant in a sense Design Systems use) and how to use it
    3. Introducing a workflow-specific (implementation-specific) way that is not a standard in developing Design Systems

    And most importantly - do not expose the variant name as a prop to the component's Twig.

    Because of this rarity, I am afraid than 99% of the time the proposed syntax will be a convoluted way of repeating an already declared enum:

    name:  Card
    variants:
      primary:
        title: Primary
        description: ...
        props:
           variant: primary
      secondary:
        title: Secondary
        description: ...
        props:
           variant: secondary
      tertiary:
        title: Tertiary
        description: ...
        props:
           variant: tertiary
     ...
    props: 
      type: object
      properties:
        variant:
          title: Variant
         type: string
        enum: 
          - primary
         - secondary
        - tertiary
    slots: {}
    

    We have primary keyword 3 times in this example, where a single time would be enough.

    Are we OK to push this verbose developer experience to component authors? I am genuinely asking, I am not pushing my own opinion here. Is the benefit (expressing variants made of multiple enums) worth the added complexity?

    Also, this is introducing a whole new layer of checks and logic. For exmaple, what will happen if the value of variants.<name>.props.<prop> is not in the prop enum or doesn't comply with the prop schema?

    Also, it is not properly in the scope of this issue, but it is worth talking about it because (contrary to Storybook usage) the definition format is not only for development tools at buildtime but also to be leverage at runtime by Drupal. What will be the UI of display building tools ( ui_patterns → , experience_builder → , display_builder → ...)? The variant selector will update the the props values? We unset the selected variant if one the prop value change?

    I propose this schema (similar to what Storybook does):

    I know Storybook is a popular (but unfortunately bloated) development tool, so I agree it is interesting to get inspiration from it. Do you have documentation about this? I don't find it. Maybe it is from an extension. Which one?

  • 🇦🇺Australia alex.skrypnyk Melbourne

    At the start, I was confused about what exactly we meant by “variants” in the context of SDC. Coming from a Storybook and design systems background, I’ve always thought of variants as examples—preset configurations of a component used for showcasing or documentation purposes. So when I saw variant logic being added to Twig templates, I was concerned it was mixing non-production/demo logic into production code.

    But after going through the discussion and listening to everyone’s perspectives, I realised that in Drupal’s case, variants are actually meant to be named bundles of prop values—things like “primary” or “pill”—used in real, production components. They help simplify implementation and support design system consistency. I now understand that these aren’t just demo examples—they’re useful, structured, and production-ready ways of managing component styles.

    I still think “variant” is a vague term—it doesn’t say what is varying—but I get that it’s widely used across design systems and frameworks. I’d personally prefer something like “kind” or “set,” but I’m happy we had the conversation, and I learned a lot from it. Thanks to everyone for the patience and thoughtful input.

  • 🇦🇺Australia alex.skrypnyk Melbourne

    I've updated https://www.drupal.org/project/drupal/issues/3522644 ✨ Add component variants to SDC, storybook style Active to use the term "examples" for clarity.

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Updating credits

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Couple of comments about the schema - I think we're missing `required`

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
  • Pipeline finished with Success
    9 days ago
    Total: 641s
    #489681
    • larowlan → committed d3aa85c8 on 11.x
      Issue #3390712 by penyaskito, pdureau, nod_, e0ipso, alex.skrypnyk,...
  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    Thanks for the quick turnaround on the CR changes @alex.skrypnyk and the MR changes @penyaskito

    Committed to 11.x and pushed.

    Published the change record.

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    I'm guessing this might be eligible for 11.2.0 release highlights, feel free to remove tag if I'm wrong.

  • 🇯🇴Jordan Rajab Natshah Jordan

    Thank you so much for this :)

    Seems that variants in SDC will be in Drupal ~11.2.0 NOT in Drupal ~11.1.0
    Should we test with the 11.x-dev for this at this time?

  • 🇺🇸United States mherchel Gainesville, FL, US

    Any chance this can be backported to 10.x? It'd be nice to use this features and have themes be compatible with both D10 and D11. My understanding is that XB hopes to support D10 also.

  • 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10

    XB is already 11.1+ only and will soon be 11.2+ only as there are important fixes to block config schemas in 11.2 that it has workarounds for.

    Without a backport do SDCs that work in 11.2+ error in 11.1 because of schema validation issues?

  • 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺

    No, but you would need to have an extra "variant" prop, and its value would have precedence over the new "variant" higher level property (we did that on purpose for BC).
    So even if not breaking anything, might prevent people that need to support 10.x to not adopt this yet.

Production build 0.71.5 2024