Skip to content

Conversation

smnandre
Copy link
Member

@smnandre smnandre commented Apr 9, 2024

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

{# 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

{# 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" />

@carsonbot carsonbot added the Status: Needs Review Needs to be reviewed label Apr 9, 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.

Can you add a test that proves "false" works as well?

@carsonbot carsonbot added Status: Needs Work Additional work is needed Status: Needs Review Needs to be reviewed and removed Status: Needs Review Needs to be reviewed Status: Needs Work Additional work is needed labels Apr 9, 2024
@smnandre smnandre requested a review from kbond April 9, 2024 20:12
@kbond kbond mentioned this pull request Apr 9, 2024
2 tasks
Copy link
Member

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

Perfect 👀

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Apr 9, 2024
@kbond kbond force-pushed the feat/cva-boolean-variants branch from dcd5ec3 to 12e0a4c Compare April 9, 2024 22:34
@kbond
Copy link
Member

kbond commented Apr 9, 2024

Thank you Simon.

@kbond kbond merged commit 3ce9987 into symfony:2.x Apr 9, 2024
@cavasinf
Copy link
Contributor

cavasinf commented Apr 10, 2024

Did you test for compoundVariants and defaultVariants with 'true' or true value ?

Like:

{% set button = cva({
  base: 'button',
  variants: {
      color: {
          blue: 'btn-blue',
          green: 'btn-red',
      },
      disabled: {
          true: 'disabled',
      }
  },
  compoundVariants: [
      // Applied via:
      //   `button({ color: 'blue', disabled: true })`
      {
          color: 'blue',
          disabled: 'true', // <-- This one 
          class: 'btn-outline-secondary',
      },
  ],
  defaultVariants: {
      disabled: 'false', // <-- This one 
  }
}) %}

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

Discussion
----------

[Twig] add additional CVA boolean tests

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| Issues        | Fix #1710 (comment)
| License       | MIT

Some additional tests for using booleans with compounds and defaultVariables.

Commits
-------

8a49bc9 [Twig] add additional CVA boolean tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TwigComponent] CVA - boolean variants not working
5 participants