Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce CVA to style TwigComponent #1416

Merged
merged 1 commit into from Feb 29, 2024

Conversation

WebMamba
Copy link
Collaborator

@WebMamba WebMamba commented Jan 23, 2024

Q A
Bug fix? no
New feature? yes
Issues
License MIT

This PR introduces a new concept CVA (Class Variance Authority), by adding a1 twig function, to help you manage your class in your component.

Let's take an example an Alert component. In your app, an alert can have a lot of different styles one for success, one for alert, one for warning, and different sizes, with icons or not... You need something that lets you completely change the style of your component without creating a new component, and without creating too much complexity in your template.

Here is the reason came CVA.

Your Alert component can now look like this:

{% props color = 'blue', size = 'md' %}

{% set alert =  cva({
     base: 'alert rounded-lg',
     variants: {
        color: {
            blue: 'text-blue-800 bg-blue-50 dark:bg-gray-800 dark:text-blue-400',
            red: 'text-red-800 bg-red-50 dark:bg-gray-800 dark:text-red-400',
            green: 'text-green-800 bg-green-50 dark:bg-gray-800 dark:text-green-400',
            yellow: 'text-yellow-800 bg-yellow-50 dark:bg-gray-800 dark:text-yellow-400',
        },
        size: {
            sm: 'px-4 py-3 text-sm',
            md: 'px-6 py-4 text-base',
            lg: 'px-8 py-5 text-lg',
        }
    },
    compoundVariants: [{
           color: ['red'],
           size: ['lg'],
          class: 'font-semibold'
    }],
    defaultVariants: {
           rounded: 'md'
    }
}) %}

<div class="{{ cva.apply({color, size}, attribute.render('class'), 'flex p-4') }}">
    ...
</div>

So here you have a cva function that lets you define different variants of your component.

You can now use your component like this:

<twig:Alert color="red" size="md"/>

<twig:Alert color="green" size="sm"/>

<twig:Alert color="yellow" size="lg"/>

<twig:Alert color="red" size="md" class="dark:bg-gray-800"/>

And then you get the following result:

Capture d’écran 2024-01-24 à 00 52 33

If you want to know more about the concept I implement here you can look at:

Tell me what you think about it! Thanks for your time! Cheers 🧡

@kbond
Copy link
Member

kbond commented Jan 24, 2024

Can you show what twm and clm stand for?

Is there resources to provide some context on cva, twm, and clm?

Copy link
Contributor

@Kocal Kocal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good :D

src/TwigComponent/src/Twig/ComponentExtension.php Outdated Show resolved Hide resolved
Comment on lines 3 to 18
{{ cva('alert', {
'base': 'rounded-lg',
'variants': {
'color': {
'blue': 'text-blue-800 bg-blue-50 dark:bg-gray-800 dark:text-blue-400',
'red': 'text-red-800 bg-red-50 dark:bg-gray-800 dark:text-red-400',
'green': 'text-green-800 bg-green-50 dark:bg-gray-800 dark:text-green-400',
'yellow': 'text-yellow-800 bg-yellow-50 dark:bg-gray-800 dark:text-yellow-400',
},
'size': {
'sm': 'px-4 py-3 text-sm',
'md': 'px-6 py-4 text-base',
'lg': 'px-8 py-5 text-lg',
}
}
}) }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{{ cva('alert', {
'base': 'rounded-lg',
'variants': {
'color': {
'blue': 'text-blue-800 bg-blue-50 dark:bg-gray-800 dark:text-blue-400',
'red': 'text-red-800 bg-red-50 dark:bg-gray-800 dark:text-red-400',
'green': 'text-green-800 bg-green-50 dark:bg-gray-800 dark:text-green-400',
'yellow': 'text-yellow-800 bg-yellow-50 dark:bg-gray-800 dark:text-yellow-400',
},
'size': {
'sm': 'px-4 py-3 text-sm',
'md': 'px-6 py-4 text-base',
'lg': 'px-8 py-5 text-lg',
}
}
}) }}
{% cva('alert', {
'base': 'rounded-lg',
'variants': {
'color': {
'blue': 'text-blue-800 bg-blue-50 dark:bg-gray-800 dark:text-blue-400',
'red': 'text-red-800 bg-red-50 dark:bg-gray-800 dark:text-red-400',
'green': 'text-green-800 bg-green-50 dark:bg-gray-800 dark:text-green-400',
'yellow': 'text-yellow-800 bg-yellow-50 dark:bg-gray-800 dark:text-yellow-400',
},
'size': {
'sm': 'px-4 py-3 text-sm',
'md': 'px-6 py-4 text-base',
'lg': 'px-8 py-5 text-lg',
}
}
}) %}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much harder to do but yes maybe I gonna be have to do it that way

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it much harder to do? (it's not a trap, that's a real question :d)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you need any help @WebMamba poke me, but i'd also prefer we keep Twig standard concerning tags/functions, ... and if your CSV does not echo anything i would be better to use a tag.

}
}) }}

<div class="{{ twm('alert', {color, size}, class) }}">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't you think twm (for Tailwind Merge) and clm (for Classes Merge) should only accept classes as parameter, and not the variant name/values? (this is what https://github.com/dcastil/tailwind-merge#tailwind-merge does)

IMO those functions have too much responsability.

Suggested change
<div class="{{ twm('alert', {color, size}, class) }}">
{% set styles_alert = cva({
'base': 'rounded-lg',
'variants': {
'color': {
'blue': 'text-blue-800 bg-blue-50 dark:bg-gray-800 dark:text-blue-400',
'red': 'text-red-800 bg-red-50 dark:bg-gray-800 dark:text-red-400',
'green': 'text-green-800 bg-green-50 dark:bg-gray-800 dark:text-green-400',
'yellow': 'text-yellow-800 bg-yellow-50 dark:bg-gray-800 dark:text-yellow-400',
},
'size': {
'sm': 'px-4 py-3 text-sm',
'md': 'px-6 py-4 text-base',
'lg': 'px-8 py-5 text-lg',
}
}
}) %}
<div class="{{ twm(cva(styles_alert, { color, size }), class, class2, class3) }}">

Here, cva({}) returns an instance of CVA, and if you pass it back to cva (cva(styles_alert, { color, size }), then it will render your CSS classes.

WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that at first but Ryan didn't like it 😅

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If using strings does it become an option to pass multiple cva names?
Like:

<div class="{{ twm('alert other_cva', {color, size, variant_from_other_cva}, class)">

Or is this too advanced for now?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about the main question here and after some thinking I would say I agree, in this implementation twm() is only usable together with cva, because it always expect a cva as first parameter, which not only combines two things that are independant from each other, it also means anyone who just wants to use some tw classes in a much simpler component can't use this twm to merge tw classes, but has to use cva with base styles for that.

I wonder what Ryan didn't like about separating those two steps 🤔
After some thinking, what about something like this:

{% set other_classes = 'm-4 grid grid-rows-[1fr]' %}
{% set alert = cva({
    'base': 'rounded-lg',
    'variants': {
        'color': {
            'blue': 'text-blue-800 bg-blue-50 dark:bg-gray-800 dark:text-blue-400',
            'red': 'text-red-800 bg-red-50 dark:bg-gray-800 dark:text-red-400',
            'green': 'text-green-800 bg-green-50 dark:bg-gray-800 dark:text-green-400',
            'yellow': 'text-yellow-800 bg-yellow-50 dark:bg-gray-800 dark:text-yellow-400',
        },
        'size': {
            'sm': 'px-4 py-3 text-sm',
            'md': 'px-6 py-4 text-base',
            'lg': 'px-8 py-5 text-lg',
        }
    }
}) %}

{% set alert_classes = cvapply(alert, { color, size }) %}
<div class="{{ twm(altert_classes, other_classes, more_tw_classes) }}">

It allows for more flexible usage, one could also use multiple cva, get the classes based on params and then throw all of them into twm, which allows some kind of composing, WDYT?

src/TwigComponent/src/Twig/ComponentExtension.php Outdated Show resolved Hide resolved
@norkunas
Copy link
Contributor

I'd probably better have something like https://styled-components.com/ or https://emotion.sh/docs/introduction because now it looks like that ux is going all tailwind 😛

@WebMamba
Copy link
Collaborator Author

@kbond description updated! 😁

I'd probably better have something like https://styled-components.com/ or https://emotion.sh/docs/introduction because now it looks like that ux is going all tailwind 😛

Yes, go LASTstack! No, you can actually use all of this without tailwind by using clminstead of twmbut yeah I think you have to architecture your style in a class-utility way. I don't believe in styled-components and emotion because you write css in to string so you don't have proper autocomplete, and it requires another heavy build step.

@norkunas
Copy link
Contributor

@kbond description updated! 😁

I'd probably better have something like https://styled-components.com/ or https://emotion.sh/docs/introduction because now it looks like that ux is going all tailwind 😛

Yes, go LASTstack! No, you can actually use all of this without tailwind by using clminstead of twmbut yeah I think you have to architecture your style in a class-utility way. I don't believe in styled-components and emotion because you write css in to string so you don't have proper autocomplete, and it requires another heavy build step.

Objects could be allowed instead of string :) no need for build step,as css could be inlined into head

@kbond
Copy link
Member

kbond commented Jan 24, 2024

What about the following syntax:

{% props color = 'blue', size = 'md' %}

<div{{ attributes.defaults({
    class: cva('alert', {
        'base': 'rounded-lg',
        'variants': {
            'color': {
                'blue': 'text-blue-800 bg-blue-50 dark:bg-gray-800 dark:text-blue-400',
                'red': 'text-red-800 bg-red-50 dark:bg-gray-800 dark:text-red-400',
                'green': 'text-green-800 bg-green-50 dark:bg-gray-800 dark:text-green-400',
                'yellow': 'text-yellow-800 bg-yellow-50 dark:bg-gray-800 dark:text-yellow-400',
            },
            'size': {
                'sm': 'px-4 py-3 text-sm',
                'md': 'px-6 py-4 text-base',
                'lg': 'px-8 py-5 text-lg',
            }
        }
    }).apply(color, size),
})>
    ...
</div>

clm/twm functions wouldn't exist but be applied automatically.

@WebMamba
Copy link
Collaborator Author

I am not sure about this syntax, because inside your div you can have other elements to tag with recipes and things can get really big and confusing. If I made two functions it's because I want the style to be separate from html, to make everything more readable.

@WebMamba
Copy link
Collaborator Author

Objects could be allowed instead of string :) no need for build step,as css could be inlined into head

Actually, the idea is not to make the perfect PR to deal with your component style, but to give utilities to manage your class. Maybe in another PR we can think about CSS in components, but this is not the topic of this PR. And actually don't believe in CSS in components but you can still convince me 😁

@norkunas
Copy link
Contributor

Actually, the idea is not to make the perfect PR to deal with your component style, but to give utilities to manage your class. Maybe in another PR we can think about CSS in components, but this is not the topic of this PR.

Yes, sorry, just wanted to bring this in 😁

And actually don't believe in CSS in components but you can still convince me 😁

Why not ? :) if you don't use css frameworks, then it would be nice to have "non detached" css from components :) and that could work for every element not only the components :)

@WebMamba
Copy link
Collaborator Author

WebMamba commented Jan 25, 2024

I am playing with the ideas from @CMH-Benny #1416 (comment) and @kbond and @Kocal
What do you think of that:

{% props color = 'blue', size = 'md', class = '' %}

{% set alert =  cva('alert', {
    'base': 'rounded-lg',
    'variants': {
        'color': {
            'blue': 'text-blue-800 bg-blue-50 dark:bg-gray-800 dark:text-blue-400',
            'red': 'text-red-800 bg-red-50 dark:bg-gray-800 dark:text-red-400',
            'green': 'text-green-800 bg-green-50 dark:bg-gray-800 dark:text-green-400',
            'yellow': 'text-yellow-800 bg-yellow-50 dark:bg-gray-800 dark:text-yellow-400',
        },
        'size': {
            'sm': 'px-4 py-3 text-sm',
            'md': 'px-6 py-4 text-base',
            'lg': 'px-8 py-5 text-lg',
        }
    }
}) }} %}

<div class="{{ alert.apply({color, size})|tw_merge }}">
    ...
</div>

@Kocal
Copy link
Contributor

Kocal commented Jan 25, 2024

That's really great, I prefer this solution!

Also, do you need to pass 'alert' as cva() 1st parameter?

@CMH-Benny
Copy link

CMH-Benny commented Jan 25, 2024

I am playing with the ideas from @CMH-Benny #1416 (comment) and @kbond and @Kocal What do you think of that:

{% props color = 'blue', size = 'md', class = '' %}

{% set alert =  cva('alert', {
    'base': 'rounded-lg',
    'variants': {
        'color': {
            'blue': 'text-blue-800 bg-blue-50 dark:bg-gray-800 dark:text-blue-400',
            'red': 'text-red-800 bg-red-50 dark:bg-gray-800 dark:text-red-400',
            'green': 'text-green-800 bg-green-50 dark:bg-gray-800 dark:text-green-400',
            'yellow': 'text-yellow-800 bg-yellow-50 dark:bg-gray-800 dark:text-yellow-400',
        },
        'size': {
            'sm': 'px-4 py-3 text-sm',
            'md': 'px-6 py-4 text-base',
            'lg': 'px-8 py-5 text-lg',
        }
    }
}) }} %}

<div class="{{ alert.apply({color, size})|tw_merge }}">
    ...
</div>

I like it!
You know what, if tw_merge becomes a filter instead of a function, you could even leave it out, so people that don't use tailwind don't have that dependency without a choice but if they tailwind, they could install https://github.com/tales-from-a-dev/twig-tailwind-extra It makes it harder to setup, but it would be more modual. Just a thought :)

What do you think?

I have another question, if i do:

{{ alert.apply({color, size})|tw_merge }}

it will automatically pass it as key value pairs and would alert.apply({color}) also work if I don't want the size classes and alert.apply({size}) if I only want the size classes?

I am not deep into the twig syntax, to me {} is an assoc array notation and I am not aware if it's similar to JS where you can put a variable { x, y, z } and it will automatically become { x: x, y: y, z: z } - So is:
alert.apply({color}) === alert.apply({color: color}) ?

@Kocal
Copy link
Contributor

Kocal commented Jan 25, 2024

I am not deep into the twig syntax, to me {} is an assoc array notation and I am not aware if it's similar to JS where you can put a variable { x, y, z } and it will automatically become { x: x, y: y, z: z } - So is: alert.apply({color}) === alert.apply({color: color}) ?

Yus, like JavaScript { color } expands to { color: color }.

@WebMamba
Copy link
Collaborator Author

Also, do you need to pass 'alert' as cva() 1st parameter?

No, it's typo

t makes it harder to setup, but it would be more modual. Just a thought :) What do you think?

I am agree. 😁

I gonna try do to a new proposal this week-end

@weaverryan
Copy link
Member

Hi!

Great iterating here! If I understand correctly, the currently-agreed-upon version that @WebMamba is going to work on is this:

{% props color = 'blue', size = 'md' %}

{% set alert =  cva('alert', {
    base: 'rounded-lg',
    variants: {
        color: {
            blue: 'text-blue-800 bg-blue-50 dark:bg-gray-800 dark:text-blue-400',
            red: 'text-red-800 bg-red-50 dark:bg-gray-800 dark:text-red-400',
            green: 'text-green-800 bg-green-50 dark:bg-gray-800 dark:text-green-400',
            yellow: 'text-yellow-800 bg-yellow-50 dark:bg-gray-800 dark:text-yellow-400',
        },
        size: {
            sm: 'px-4 py-3 text-sm',
            md: 'px-6 py-4 text-base',
            lg: 'px-8 py-5 text-lg',
        }
    }
}) }} %}

<div class="{{ alert.apply({color, size})|tw_merge }}">
    ...
</div>

(note, I removed all of the ' in the keys - they're not needed, and this looks better 😉 ).

If so, yes, I like this! But 2 things remain:

A) How can this work with attributes? Like this:

<div {{ attributes.defaults({
    class: alert.apply({color, size})|tw_merge
}) }}>

B) And related, instead of creating a new tw_merge function, can we just tell people to use https://github.com/tales-from-a-dev/twig-tailwind-extra? AND, if that package is installed, could we automatically "merge" the class attribute on all ComponentAttributes? I think this may be tricky from an implementation standpoint, but if it's the best API, we should make it happen.

@weaverryan weaverryan added Status: Needs Work Additional work is needed Feature New Feature labels Jan 29, 2024
@kbond
Copy link
Member

kbond commented Jan 29, 2024

And related, instead of creating a new tw_merge function, can we just tell people to use https://github.com/tales-from-a-dev/twig-tailwind-extra?

+1 to this

@WebMamba
Copy link
Collaborator Author

WebMamba commented Jan 29, 2024

Hey! Thanks everyone for all of your ideas! So here is the details of what I did in my last commit:

{% props color = 'blue', size = 'md', class = '' %}

{% set alert = cva({
    base: 'alert',
    variants: {
        color: {
            blue: 'alert-blue',
            red: 'alert-red',
            green: 'alert-green',
            yellow: 'alert-yellow',
        },
        size: {
            sm: 'alert-sm',
            md: 'alert-md',
            lg: 'alert-lg',
        }
    },
    compounds: [
        {
            color: ['red'],
            size: ['lg'],
            class: 'font-semibold'
        }
    ]
}) %}

<div {{ attributes.defaults({
    class: alert.apply({color, size}, class)
}) }}>
    ...
</div>

Then you can use your component like so:

<twig:Alert2 color='red' size='lg' class='dark:bg-gray-600'/>

And you will get the following result:

<div  class="alert alert-red alert-lg font-semibold dark:bg-gray-600">
    ...
</div>

I didn't introduce compounds in my description but the idea is if the condition in the compounds are fullfield then the classes are apply.

@weaverryan weaverryan added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Jan 30, 2024
@CMH-Benny
Copy link

CMH-Benny commented Jan 31, 2024

Looks pretty good to me 👍

It says it needs review - I am pretty new to follow PRs like this 😅
Is there anything a rando like me can do to help to move forward or will this be reviewed as soon as somebody from the maintainers finds the time?

Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this! Few things I'd like to discuss:

  1. CVA: is this a commonly known acronym? It feels a bit arbitrary to me anyway. I'd prefer it be expanded out.
  2. Since this isn't really twig-component specific (I don't think), should we at least try and get this into twig/html-extra? I know that is easier said than done.

@smnandre
Copy link
Collaborator

smnandre commented Feb 1, 2024

I need to insist on that point, but if cva does not "output" something, it should be either a tag or in this case... a filter

Most of the time though, a tag is not needed:
* If your tag generates some output, use a function instead.
* If your tag modifies some content and returns it, use a filter instead.
For instance, if you want to create a tag that converts a Markdown formatted text to HTML, create a markdown filter instead: (...)

https://twig.symfony.com/doc/3.x/advanced.html#tags

So it should be

{% set alert = {
    base: 'alert',
    variants: {
        color: {
            blue: 'alert-blue',
            red: 'alert-red',
            green: 'alert-green',
            yellow: 'alert-yellow',
        },
        size: {
            sm: 'alert-sm',
            md: 'alert-md',
            lg: 'alert-lg',
        }
    },
    compounds: [
        {
            color: ['red'],
            size: ['lg'],
            class: 'font-semibold'
        }
    ]
}|cva %}

:)

@CMH-Benny
Copy link

CMH-Benny commented Feb 1, 2024

Somehow I can't reply directly, so I need to send a new comment 🙈

I need to insist on that point, but if cva does not "output" something, it should be either a tag or in this case... a filter

Most of the time though, a tag is not needed:

  • If your tag generates some output, use a function instead.
  • If your tag modifies some content and returns it, use a filter instead.
    For instance, if you want to create a tag that converts a Markdown formatted text to HTML, create a markdown filter instead: (...)

https://twig.symfony.com/doc/3.x/advanced.html#tags

So it should be

{% set alert = {
    base: 'alert',
    variants: {
        color: {
            blue: 'alert-blue',
            red: 'alert-red',
            green: 'alert-green',
            yellow: 'alert-yellow',
        },
        size: {
            sm: 'alert-sm',
            md: 'alert-md',
            lg: 'alert-lg',
        }
    },
    compounds: [
        {
            color: ['red'],
            size: ['lg'],
            class: 'font-semibold'
        }
    ]
}|cva %}

:)

But it outputs something now, it's a CVA object you call apply on to output the fitting classes based on given properties, so a function now makes sense? Otherwise you would have to do:

{% props color='blue', size = 'md', class = '' %}
{% set alert = {
     base: 'alert',
     variants: {
         color: {
             blue: 'alert-blue',
             red: 'alert-red',
             green: 'alert-green',
             yellow: 'alert-yellow',
         },
         size: {
             sm: 'alert-sm',
             md: 'alert-md',
             lg: 'alert-lg',
         }
     },
     compounds: [
         {
             color: ['red'],
             size: ['lg'],
             class: 'font-semibold'
         }
     ]
}|cva({color, size}, class) %}

And I am not sure if that works, "alert" becomes a loose object and how can the CVA now parse it and apply the correct variants? I think the provided solution looks good as it is, or do I miss anything? @smnandre

@smnandre
Copy link
Collaborator

smnandre commented Feb 1, 2024

Alert is an CVA instance in both cases

{# cva function #}
{% set alert = cva({foo: bar}) %}

{# cva filter #}
{% set alert = {foo: bar}|cva %}

Those two syntaxes happen to give the same result. But the implementation is not the same, the optimisations neither, and ... one does not follow Twig (or Symfony) conventions.

@CMH-Benny
Copy link

Oh, now I get it - I learned something new again!

So by "output" you don't mean it doesn't return anything, it means it doesn't directly add markup/text to the final output

So if it's a function it needs to be used in {{ cva(...) }}
But since it's only providing an object that needs to get applied() to actually create output, it should become a filter!
So the full example would look like this:

{% props color='blue', size = 'md', class = '' %}
{% set alert = {
     base: 'alert',
     variants: {
         color: {
             blue: 'alert-blue',
             red: 'alert-red',
             green: 'alert-green',
             yellow: 'alert-yellow',
         },
         size: {
             sm: 'alert-sm',
             md: 'alert-md',
             lg: 'alert-lg',
         }
     },
     compounds: [
         {
             color: ['red'],
             size: ['lg'],
             class: 'font-semibold'
         }
     ]
}|cva %}

<div {{ attributes.defaults({
    class: alert.apply({color, size}, class)
}) }}>
    ...
</div>

<div {{ alert.apply({color, size}, 'some other classes') }}>...</div>

@smnandre
Copy link
Collaborator

smnandre commented Feb 1, 2024

Exactly !

@kbond
Copy link
Member

kbond commented Feb 2, 2024

Best practices aside, this reads better to me:

{% set alert = cva({
    base: 'alert',
    variants: {
        color: {
            blue: 'alert-blue',
            red: 'alert-red',
            green: 'alert-green',
            yellow: 'alert-yellow',
        },
        size: {
            sm: 'alert-sm',
            md: 'alert-md',
            lg: 'alert-lg',
        }
    },
    compounds: [
        {
            color: ['red'],
            size: ['lg'],
            class: 'font-semibold'
        }
    ]
}) %}

Than:

{% set alert = {
    base: 'alert',
    variants: {
        color: {
            blue: 'alert-blue',
            red: 'alert-red',
            green: 'alert-green',
            yellow: 'alert-yellow',
        },
        size: {
            sm: 'alert-sm',
            md: 'alert-md',
            lg: 'alert-lg',
        }
    },
    compounds: [
        {
            color: ['red'],
            size: ['lg'],
            class: 'font-semibold'
        }
    ]
}|cva %}

But maybe that's just me.

@kbond
Copy link
Member

kbond commented Feb 28, 2024

Thanks, I gonna make some changes to match the original name

If you use these camelCase names (compoundVariants & defaultVariants), using twig named parameters is going to suffer from twigphp/Twig#3475. Maybe this would just be a documentation thing but my preference would be to use an array only. If in the future, named arguments are fixed/improved in twig, we should be able to switch w/o BC break.

@WebMamba
Copy link
Collaborator Author

OK, thanks @kbond! I am on it

Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting close I think - just a few suggestions.

src/TwigComponent/src/Twig/ComponentExtension.php Outdated Show resolved Hide resolved
src/TwigComponent/src/Twig/ComponentExtension.php Outdated Show resolved Hide resolved
src/TwigComponent/src/CVA.php Outdated Show resolved Hide resolved
src/TwigComponent/src/CVA.php Outdated Show resolved Hide resolved
src/TwigComponent/doc/index.rst Outdated Show resolved Hide resolved
src/TwigComponent/doc/index.rst Outdated Show resolved Hide resolved
src/TwigComponent/doc/index.rst Outdated Show resolved Hide resolved
src/TwigComponent/doc/index.rst Outdated Show resolved Hide resolved
@carsonbot carsonbot added Status: Needs Work Additional work is needed and removed Status: Needs Review Needs to be reviewed labels Feb 28, 2024
@WebMamba
Copy link
Collaborator Author

Hey all! I updated the description of the PR but here is what the last commit brings:
So now the CVA function has just one argument a big array key:

  • base: to define all the classes that are present in all variations, it's a string
  • variants: your different variants
  • compoundVariants: let you the ability to apply a set of classes only if a specific set of variants match
  • defaultVariants: let you define a default variant to pick if none of the variations match

The function `apply can accept has many arguments you want to give you the ability to add as many sets of class as you want.
And I made some renaming.
Cheers 🧡

@carsonbot carsonbot added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Feb 28, 2024
Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Feb 29, 2024
src/TwigComponent/CHANGELOG.md Show resolved Hide resolved
src/TwigComponent/CHANGELOG.md Show resolved Hide resolved
src/TwigComponent/doc/index.rst Show resolved Hide resolved
src/TwigComponent/doc/index.rst Show resolved Hide resolved
src/TwigComponent/doc/index.rst Show resolved Hide resolved
Default variants
~~~~~~~~~~~~~~~~

You can define defaults variants, so if no variants are matching you
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
You can define defaults variants, so if no variants are matching you
If no variants match, you can define a default set of classes to apply:

.. code-block:: html+twig

{# templates/components/Alert.html.twig #}
{% props color = 'blue', size = 'md' %}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need rounded = '' up here?

And if so, then does defaultVariants have limited usage? Not that we should remove it, just clarifying :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CVA can be use outside of a component this is why we may need it. What do you think ?

lg: 'rounded-lg',
}
},
defaultsVariants: {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
defaultsVariants: {
defaultVariants: {

@@ -1058,6 +1058,198 @@ Exclude specific attributes:
My Component!
</div>

Component with Complex Variants (CVA)
-------------------------------------
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need a versionadded for 2.16

{
$recipeClass = new CVA($recipe['base'] ?? '', $recipe['variants'] ?? [], $recipe['compounds'] ?? [], $recipe['defaultVariants'] ?? []);

$this->assertEquals($expected, $recipeClass->resolve($recipes));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a basic test with ->apply() that shows the classes being appending properly and trimmed. It's tested via the Twig function test, but better to have a simple one here too.

@weaverryan
Copy link
Member

Thank you for this huge work Matheo! I've left some minor comments and docs comments for a future PR please :). But let's get this merged and in there

@weaverryan weaverryan merged commit 772fea9 into symfony:2.x Feb 29, 2024
@CMH-Benny
Copy link

Wooohoo, it's merged 🎉

Thank you everyone for your efforts and the amazing features you provide to us developers ❤️

weaverryan added a commit that referenced this pull request Feb 29, 2024
This PR was merged into the 2.x branch.

Discussion
----------

Prepping 2.16 changelogs

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| Issues        | None
| License       | MIT

Time for our end-of-month release.

And since the last release 9 days ago... this is packed!

* ✅ Live Components stable
* ✅ CVA #1416

... and many other goodies and fixes. Cool 😎

This release DOES contain a few BC breaks in Live Components (the last ones!) that will affect many / most applications. See the CHANGELOG for LiveComponents for details.

Commits
-------

53ac573 Prepping 2.16 changelogs
@WebMamba
Copy link
Collaborator Author

WebMamba commented Mar 1, 2024

Thanks a lot, everyone for participating in this PR! I wish you a lot of fun with this cool new feature! 🧡

@Kocal
Copy link
Contributor

Kocal commented Mar 1, 2024

Great! :D

Thanks everyooone :)

weaverryan added a commit that referenced this pull request Mar 1, 2024
This PR was merged into the 2.x branch.

Discussion
----------

Update CVA docs and fixes

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| Issues        |
| License       | MIT

`@weaverryan` asked for a few changes here: #1416.
So here are the fixes. 😁

Commits
-------

8567c68 [Twig] Fixing docs syntax in one more spot
75fa44f [Twig] Fixing rst docs syntax
08a94f1 [Twig] Minor docs tweaks
6b3a8ea Update src/TwigComponent/doc/index.rst
6f49dc1 Compound variant
057d68b Update CVA docs and fixes
@seb-jean
Copy link
Contributor

seb-jean commented Mar 4, 2024

@WebMamba How could I handle booleans?

Here is an example but it doesn't work

{# templates/components/Avatar.html.twig #}

{% props square = false, src, alt = null %}

 {% set avatar = cva({
     base: 'inline-grid align-middle *:col-start-1 *:row-start-1 ',
     variants: {
         square: {
             true: 'rounded-[20%] *:rounded-[20%]',
             false: 'rounded-full *:rounded-full',
         },
     }
 }) %}

@WebMamba
Copy link
Collaborator Author

WebMamba commented Mar 4, 2024

Hey @seb-jean, the following code should work:
avartar.apply({square: square ? 'true' : 'false'})

@seb-jean
Copy link
Contributor

seb-jean commented Mar 4, 2024

Yeah !! Thanks @WebMamba 🏀 !

@seb-jean
Copy link
Contributor

seb-jean commented Mar 6, 2024

But is this the best way to handle boolean variants?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New Feature Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants