[Toolkit][Shadcn] Remove attributes.defaults() in component templates

[seb-jean]

Q	A
Bug fix?	no
New feature?	no
Deprecations?	no
Documentation?	yes
Issues	Part of #3233
License	MIT

[Kocal]

Thank you for working on this, but there are a few nuances to consider. I stopped the review at src/Toolkit/kits/shadcn/alert/templates/components/Alert.html.twig file btw

[Kocal]

However, for data-action it can be different, maybe it can be useful to make it overridable, but only by using the html_attr_merge Twig filter

[seb-jean]

Thanks for your feedback, @Kocal

1. I'm wondering, then, what is the purpose of attributes.defaults({})? Is there an advantage? Should we avoid using attributes.defaults({})?

2. With the 3.x branch update, I changed the template in src/Toolkit/kits/shadcn/label/examples/Demo.html.twig to add data-slot="test-data-slot".

<div class="flex gap-2">
    <twig:Checkbox id="terms" />
    <twig:Label for="terms" data-slot="test-data-slot">Accept terms and conditions</twig:Label>
</div>

I went to http://127.0.0.1:9044/toolkit/kits/shadcn/components/label, then opened a new tab to view the preview demo: http://127.0.0.1:9044/toolkit/component_preview?[...].

I looked at the source code, and I saw two data slots applied to the <label> element:

<label data-slot="label" class="[...]" for="terms" data-slot="test-data-slot">Accept terms and conditions</label>

Furthermore, this might need to be modified because it mentions the use of attributes.defaults({...}):

• https://github.com/symfony/ux/blob/3.x/.agents/skills/symfony-ux-toolkit-kit/SKILL.md#2-root-element
• https://github.com/symfony/ux/blob/3.x/.agents/skills/symfony-ux-toolkit-kit/SKILL.md#anti-patterns-flag-in-review
• https://github.com/symfony/ux/blob/3.x/.agents/skills/symfony-ux-toolkit-kit/SKILL.md#bad--good

[xDeSwa]

hi @seb-jean

can you try and update select component?

Current :

{# @block content The select options (`<option>` elements) #}
<select
    class="{{ ('flex h-10 w-full items-center justify-between rounded-md border border-input bg-background px-3 py-2 text-sm ring-offset-background data-[placeholder]:text-muted-foreground focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:cursor-not-allowed disabled:opacity-50 [&>span]:line-clamp-1 ' ~ attributes.render('class'))|tailwind_merge }}"
    {{ attributes }}
>
    {%- block content %}{% endblock -%}
</select>

Better view:

{# @block content The select options (`<option>` elements) #}
<div class="relative w-full">
    <select
        class="{{ ('flex w-full appearance-none items-center justify-between rounded-lg border bg-transparent px-3 py-2 text-sm ring-offset-background transition-colors focus-visible:outline-none focus-visible:ring-3 focus-visible:ring-ring/50 disabled:cursor-not-allowed disabled:opacity-50 dark:bg-input/30 dark:hover:bg-input/50 [&>span]:line-clamp-1 ' ~ attributes.render('class'))|tailwind_merge }}"
        {{ attributes }}
    >
        {%- block content %}{% endblock -%}
    </select>

    <div class="pointer-events-none absolute right-1 mr-2 top-0 bottom-0 flex items-center justify-center text-muted-foreground">
        <twig:ux:icon name="lucide:chevron-down" class="h-4 w-4 opacity-50" />
    </div>
</div>

[seb-jean]

hi @seb-jean

can you try and update select component?

Current :

{# @block content The select options (`<option>` elements) #}
<select
    class="{{ ('flex h-10 w-full items-center justify-between rounded-md border border-input bg-background px-3 py-2 text-sm ring-offset-background data-[placeholder]:text-muted-foreground focus:outline-none focus:ring-2 focus:ring-ring focus:ring-offset-2 disabled:cursor-not-allowed disabled:opacity-50 [&>span]:line-clamp-1 ' ~ attributes.render('class'))|tailwind_merge }}"
    {{ attributes }}
>
    {%- block content %}{% endblock -%}
</select>

Better view:

{# @block content The select options (`<option>` elements) #}
<div class="relative w-full">
    <select
        class="{{ ('flex w-full appearance-none items-center justify-between rounded-lg border bg-transparent px-3 py-2 text-sm ring-offset-background transition-colors focus-visible:outline-none focus-visible:ring-3 focus-visible:ring-ring/50 disabled:cursor-not-allowed disabled:opacity-50 dark:bg-input/30 dark:hover:bg-input/50 [&>span]:line-clamp-1 ' ~ attributes.render('class'))|tailwind_merge }}"
        {{ attributes }}
    >
        {%- block content %}{% endblock -%}
    </select>

    <div class="pointer-events-none absolute right-1 mr-2 top-0 bottom-0 flex items-center justify-center text-muted-foreground">
        <twig:ux:icon name="lucide:chevron-down" class="h-4 w-4 opacity-50" />
    </div>
</div>

I'm not going to make the change because it's outside the scope of the PR and concerns another component introduced in the PR: #3479

[seb-jean]

I’m adding two examples of attributes.defaults() usage that are contradictory.

In the file ux/src/Toolkit/kits/shadcn/alert/templates/components/Alert.html.twig, the data-slot and role attributes are not included in attributes.defaults():

ux/src/Toolkit/kits/shadcn/alert/templates/components/Alert.html.twig

Lines 14 to 21 in 08eeb19

	<div
	data-slot="alert"
	role="alert"
	class="{{ style.apply({variant: variant}, attributes.render('class'))|tailwind_merge }}"
	{{ attributes }}
	>
	{%- block content %}{% endblock -%}
	</div>

However, in the file ux/src/Toolkit/kits/shadcn/accordion/templates/components/Accordion/Content.html.twig, the data-slot and role attributes are included in attributes.defaults():

ux/src/Toolkit/kits/shadcn/accordion/templates/components/Accordion/Content.html.twig

Lines 5 to 24 in 08eeb19

	<div
	class="grid text-sm overflow-hidden transition-[grid-template-rows] duration-300 ease-out {{ not _accordionItem_isOpen ? 'hidden' }}"
	{{ attributes.defaults({
	id: _accordionItem_contentId,
	'data-slot': 'accordion-content',
	'data-accordion-target': 'content',
	role: 'region',
	'aria-labelledby': _accordionItem_triggerId,
	'aria-hidden': not _accordionItem_isOpen ? 'true' : 'false',
	'data-open': _accordionItem_isOpen ? 'true' : false,
	'data-closed': not _accordionItem_isOpen ? 'true' : false,
	style: _accordionItem_isOpen ? 'grid-template-rows: 1fr;' : 'grid-template-rows: 0fr;',
	}).without('class') }}
	>
	<div class="min-h-0 min-w-0 overflow-hidden">
	<div class="{{ ('pt-0 pb-2.5 [&_a]:underline [&_a]:underline-offset-3 [&_a]:hover:text-foreground [&_p:not(:last-child)]:mb-4 ' ~ attributes.render('class'))|tailwind_merge }}">
	{%- block content %}{% endblock -%}
	</div>
	</div>
	</div>

As a result, I’m not sure which approach is the recommended one or what the best practice is.
Should we avoid using attributes.defaults()? When should we use it?

[seb-jean]

I made multiple commits to make the review easier.

I didn't understand the comment, could you explain how to proceed?

I kept attributes.defaults() on the Label, Separator, Button, Input and Textarea components because in Shadcn, these attributes need to be overridden:

• https://github.com/shadcn-ui/ui/blob/main/apps/v4/styles/radix-nova/ui/item.tsx
• https://github.com/shadcn-ui/ui/blob/main/apps/v4/styles/radix-nova/ui/input-group.tsx
• https://github.com/shadcn-ui/ui/blob/main/apps/v4/styles/radix-nova/ui/field.tsx
• https://github.com/shadcn-ui/ui/blob/main/apps/v4/styles/radix-nova/ui/button-group.tsx