Skip to content

[Twig] boolean (true) CVA variants #1647

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

Closed
wants to merge 1 commit into from
Closed

Conversation

kbond
Copy link
Member

@kbond kbond commented Mar 22, 2024

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

@carsonbot carsonbot added the Status: Needs Review Needs to be reviewed label Mar 22, 2024
@seb-jean
Copy link
Contributor

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

@Kocal
Copy link
Member

Kocal commented Mar 22, 2024

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

@kbond
Copy link
Member Author

kbond commented Mar 22, 2024

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.

Comment on lines +56 to 64
if (isset($this->variants[$recipeName]) && (\is_string($this->variants[$recipeName]) || (\is_array($this->variants[$recipeName]) && array_is_list($this->variants[$recipeName])))) {
if (true === $recipeValue) {
$classes .= ' '.implode(' ', (array) $this->variants[$recipeName]);
}

continue;
}

if (!isset($this->variants[$recipeName][$recipeValue])) {
Copy link
Member

@smnandre smnandre Mar 22, 2024

Choose a reason for hiding this comment

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

Hmm. I fear this become hard to understand for the next ones :)

I'll try another way, you'll tell me if it's worth the change 🤷

Copy link
Member

Choose a reason for hiding this comment

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

There is something i don't understand in the algorithm will ask you questions about this, but don"t wait for me in this PR, that's 100% implementation and can be handled way after :)

@seb-jean
Copy link
Contributor

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>

@smnandre
Copy link
Member

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

@kbond
Copy link
Member Author

kbond commented Mar 25, 2024

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[]>

@smnandre
Copy link
Member

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

@cavasinf
Copy link
Contributor

cavasinf commented Apr 8, 2024

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);
    }
}

@smnandre
Copy link
Member

smnandre commented Apr 8, 2024

How is this related to CVA ?

@cavasinf
Copy link
Contributor

cavasinf commented Apr 8, 2024

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".

@smnandre
Copy link
Member

smnandre commented Apr 8, 2024

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 :)

@kbond
Copy link
Member Author

kbond commented Apr 9, 2024

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
Copy link
Member Author

kbond commented Apr 9, 2024

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

@kbond kbond closed this Apr 9, 2024
@kbond kbond deleted the twig/cva-boolean branch April 9, 2024 20:14
@Kocal
Copy link
Member

Kocal commented Apr 9, 2024

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

kbond added a commit that referenced this pull request Apr 9, 2024
This PR was squashed before being merged into the 2.x branch.

Discussion
----------

[Twig] Resolving boolean as variant keys

After some "back and forth", we agreed on the following behaviour. I hope this will match your needs :)

This PR allows users to pass real boolean when calling the component, to match string-boolean defined in the CVA definition.

Based on the work of `@kbond` in #1647  to fix #1614

```twig
{# templates/components/Button.html.twig #}

{% props color = 'blue', giant = false %}
{% set button = cva({
  base: 'button',
  variants: {
      color: {
          blue: 'btn-blue',
          green: 'btn-red',
      },
      giant: {
          true: 'btn-giant',
      }
  }
}) %}

<button class="{{ button.apply({color, giant}, attributes.render('class')) }}">
   {{ label }}
</button>
```

Then the 3 following calls should be equivalent

```twig

{# String value #}
<twig:Button color="blue" giant="true"  label="Save" />

{# Boolean value #}
<twig:Button color="blue" giant="{{ true }}"  label="Save" />

{# Boolean expression #}
<twig:Button color="blue" giant="{{ 2 > 1 }}"  label="Save" />
```

Commits
-------

12e0a4c [Twig] Resolving boolean as variant keys
@cavasinf
Copy link
Contributor

I agree with #1710 as well. 👍

@kbond kbond restored the twig/cva-boolean branch April 14, 2024 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Review Needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TwigComponent] CVA - boolean variants not working
6 participants