ux icon indicating copy to clipboard operation
ux copied to clipboard

[Twig] boolean (true) CVA variants

Open kbond opened this issue 1 year ago • 7 comments

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

This is an attempt to solve #1614. As discussed in that issue, it seems like only a true should be valid. Because of this, I opted for the variant value(s) to be a string or list<string> - this is used when the recipe value is true. Examples:

{% props size = 'md', disabled = false %}

{% set button = cva({
    variants: {
        size: {
            sm: 'alert-sm',
            md: 'alert-md',
            lg: 'alert-lg',
        },
        disabled: 'opacity-50 cursor-not-allowed',
        disabled: ['opacity-50', 'cursor-not-allowed'], {# also valid #}
    },
}) %}

<button class="{{ button.apply({size, disabled}, attributes.render('class'), 'flex p-4') }}">
    ...
</button>

TODO

  • [ ] docs
  • [ ] changelog

kbond avatar Mar 22 '24 17:03 kbond

When disabled is false, how could we associate CSS classes with it ?

seb-jean avatar Mar 22 '24 18:03 seb-jean

To me it should only accept an array with true and false as keys

Kocal avatar Mar 22 '24 18:03 Kocal

When disabled is false, how could we associate CSS classes with it ?

Yeah, you can't.

I sort of followed this (I don't know for sure that false is not supported) but simplified the implementation.

To me it should only accept an array with true and false as keys

I went down this road initially but, since a real bool cannot be an array key, I'd have to fudge 0|'false' and 1|'true' to represent false and true respectively. This felt a bit brittle to me.

kbond avatar Mar 22 '24 19:03 kbond

So, I think it would be interesting to have something like this (to make it clear 😄):

{% props size = 'md', disabled = false %}

{% set button = cva({
    variants: {
        size: {
            sm: 'alert-sm',
            md: 'alert-md',
            lg: 'alert-lg',
        },
         disabled: {
             true: 'opacity-50 cursor-not-allowed',
             false: 'bg-blue-600 text-orange-400',
         }
    },
}) %}

<button class="{{ button.apply({size, disabled}, attributes.render('class'), 'flex p-4') }}">
    ...
</button>

seb-jean avatar Mar 23 '24 10:03 seb-jean

So we're back to the suggestion: either all your "keys" are boolean, either all are strings ?

smnandre avatar Mar 24 '24 21:03 smnandre

So we're back to the suggestion: either all your "keys" are boolean, either all are strings ?

But keys can't be real booleans so we have to say "true" === true, "false" === false and/or 1 === true, 0 === false. Something like array<string,string|string[]>|array<"true"|"false",string|string[]>

kbond avatar Mar 25 '24 18:03 kbond

Do as you think it's best, and you'll have my full support :))

smnandre avatar Mar 25 '24 21:03 smnandre

https://cva.style/docs/examples/other-use-cases

cva is really just a fancy way of managing a string…

It would/should treat boolean as a string, even false.


I think it will be great if these configs work:

<twig:Alert /> {# Default value #}
<twig:Alert disabled /> {# true #}
<twig:Alert disabled="" /> {# true #}
<twig:Alert disabled="disabled" /> {# true #}
<twig:Alert disabled="anyStringValue" /> {# true #}
<twig:Alert disabled="true" /> {# true #}
<twig:Alert disabled="false" /> {# false #}
<twig:Alert disabled="{{ true }}" /> {# true #}
<twig:Alert disabled="{{ false }}" /> {# false #}
{% set trueBoolean = true %}
<twig:Alert disabled="{{ trueBoolean }}" /> {# true #}
{% set trueBoolean = true %}
<twig:Alert :disabled="trueBoolean" /> {# true #}

We achieve that by normalizing the option to a string, and anything but false or 0 is true.

#[AsTwigComponent]
final class Alert
{
    public string $disabled = 'false';

    #[PreMount]
    public function preMount(array $data): array
    {
        $resolver = new OptionsResolver();

        $resolver->setDefaults(
            [
                'disabled' => 'false',
            ]
        );

        $resolver->setAllowedTypes('disabled', ['bool', 'string']);
        $resolver->setNormalizer('disabled', function(Options $options, string|bool $value) {
            if (is_bool($value)) {
                $strBool = $value ? 'true' : 'false';
            } else {
                $strBool = $value;
            }

            return preg_match('/^(false|0)$/i', strtolower($strBool)) === 1 ? 'false' : 'true';
        });

        return $resolver->resolve($data);
    }
}

cavasinf avatar Apr 08 '24 12:04 cavasinf

How is this related to CVA ?

smnandre avatar Apr 08 '24 18:04 smnandre

Because variants cannot be true only, but 'true' AND 'false'.

cva({
    variants: {
        disabled: 'opacity-50 cursor-not-allowed',
    },
})

VS

cva({
    variants: {
         disabled: {
             true: 'opacity-50 cursor-not-allowed',
             false: 'bg-blue-600 text-orange-400',
         }
    },
})

As the doc mentioned above: https://cva.style/docs/examples/other-use-cases

So today, we have to cast boolean value/key to full letter "string boolean".

cavasinf avatar Apr 08 '24 18:04 cavasinf

The moment #1653 is merged, i'll rework this code from @kbond (yeah i changed to original code ... so i'll assume this and won't ask him to adapt this PR haha) and we'll ship an easier DX, as multiple people ask it :)

smnandre avatar Apr 08 '24 23:04 smnandre

I agree with @cavasinf in that all keys should be strings/"string booleans". I'll rework this now that #1653 is merged and we can do another round of discussion.

kbond avatar Apr 09 '24 17:04 kbond

I'm in favor of the alternate implementation in #1710. WDYT @seb-jean, @Kocal, @cavasinf?

kbond avatar Apr 09 '24 20:04 kbond

I'm also in favor of #1710, which keep the usage and maintenance very simple! :)

Kocal avatar Apr 09 '24 20:04 Kocal

I agree with #1710 as well. 👍

cavasinf avatar Apr 10 '24 08:04 cavasinf