- Issue created by @pdureau
- 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.
- 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 newMissingComponentVariantException
). 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.
- 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 forvariant
) 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 ofanyOf
withconstant
)
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:
/components//..twig because:<provider>/components/<component-name>/<component-name>--<variant>.twig
because it looks like a template suggestion, so familiar.- not a "problematic" character in filenames
- not a machine name (used in Plugin ID AFAIK) character in Drupal: https://www.drupal.org/node/2954832 →
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" } } } } }
- both of them have a predefined and fixed prop type (Attribute object for
- 🇪🇸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
- 🇪🇸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. - 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
This needs sanity checks from @e0ipso.
- Status changed to Needs review
8 months ago 7:52am 29 May 2024 - 🇫🇷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
8 months ago 8:27am 29 May 2024 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?
- 🇮🇳India 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 Kristen Pol Santa Cruz, CA, USA
Adding FAQ page that refers to variants to summary:
https://www.drupal.org/docs/develop/theming-drupal/using-single-director... →
- 🇫🇷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 🇧🇪🇪🇺
https://www.drupal.org/project/experience_builder → also needs this: 📌 [SPIKE] Comprehensive plan for integrating with SDC Active .
- 🇪🇸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', }) }}
- 🇪🇸Spain penyaskito Seville 💃, Spain 🇪🇸, UTC+2 🇪🇺
Still missing:
- Schema
- Schema validation
- Schema validation tests.