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

Compound Variants not working as expected with breakpoints #216

Closed
loiacon opened this issue Sep 15, 2020 · 6 comments
Closed

Compound Variants not working as expected with breakpoints #216

loiacon opened this issue Sep 15, 2020 · 6 comments
Assignees
Labels
bug Something isn't working P1

Comments

@loiacon
Copy link

loiacon commented Sep 15, 2020

Bug report

Describe the bug

When using compoundVariants, if we don't specify all breakpoint cases, the style will not be applied correctly.

To Reproduce

  1. Go to https://stitches.dev/docs/variants
  2. Scroll down to Compound variants section
  3. Change the last <Button> with:
    <Button color="gray" appearance={{ initial: null, bp1: 'outline' }}>
      Button
    </Button>
  1. See error

Expected behavior

The compound variant works as expected with responsive styles

Screenshots

Gray Button with outline variant:
image

Gray Button with outline variant and responsive style:
image

System information

  • OS: macOS
  • Browser: Chrome 85.0.4183.102 (64-bit)
@peduarte peduarte added the bug Something isn't working label Sep 15, 2020
@peduarte
Copy link
Contributor

@loiacon thanks for catching this bug 🐛 we'll look into it.

@hadihallak
Copy link
Member

hey @loiacon 👋
this is the expected results as the compound variant never matched for it to be applied.

Doing this:

  <Button color="gray" appearance={{ initial: null, bp1: 'outline' }}>
      Button
    </Button>

Translates to this:

  <Button color={{ initial: 'gray' }} appearance={{ initial: null, bp1: 'outline' }}>
      Button
    </Button>

and as you can see above, the compound variant isn't going to match at any breakpoint so the compound styles are never applied (for a compound styles to be applied, they need to match at the same breakpoint)

I do see how this is confusing tho -- curious if you have any idea in mind on how we should handle this.

@loiacon
Copy link
Author

loiacon commented Sep 22, 2020

Hey, @hadihallak, first of all, thank you and all the team to the great work with stitches. I'm really enjoying using it!

How about translate to all breakpoints if no one is specified?

Doing this:

<Button color="gray" appearance={{ initial: null, bp1: 'outline' }}>
  Button
</Button

Translates to this:

<Button color={{ initial: 'gray', bp1: 'gray' }} appearance={{ initial: null, bp1: 'outline' }}>
  Button
</Button>

With this approach, the library can get closer to how CSS works:

.button {
  color: gray; // this will be applied to all media queries
}
@media (...) {
  ...
}

@hadihallak
Copy link
Member

yeah, this is similar to what i had in mind. the only issue i could see with this is that this approach will lead to the creation of a lot of responsive atom classes so this might affect performance.

Maybe we could optimize it so that it only does this treatment when the variant is a part of a compound variant
Example one:

<Button color="gray" appearance={{ initial: null, bp1: 'outline' }}>
  Button
</Button>

here if both the color variant and appearance variants were part of a compound variant, the color variant will be turned into

// we add the breakpoint to color
<Button color={{ initial: 'gray', bp1: 'gray' }} appearance={{ initial: null, bp1: 'outline' }}>
  Button
</Button>

like in your example

Example two:
variants aren't part of a single compound variant

<Button color="gray" appearance={{ initial: null, bp1: 'outline' }}>
  Button
</Button>

will turn into:

// we didn't add the breakpoint to the color variant
<Button color={{ initial: 'gray' }} appearance={{ initial: null, bp1: 'outline' }}>
  Button
</Button>

Example three:
if both were part of a compound variant but non of them had responsive values applied to them like

<Button color="gray" appearance="outline">
  Button
</Button>

it will translate to

<Button color={{ initial: 'gray' }} appearance={{ initial: 'outline' }}>
  Button
</Button>

hmm... will keep this open for a few days just to think this through some more...

@loiacon
Copy link
Author

loiacon commented Sep 23, 2020

I think this bug only happens when the variant is a part of a compound variant, so it's fair to optimize this only to them.
Looks great to me your solution to this problem!

@peduarte
Copy link
Contributor

Closing in favour of @jonathantneal who has this under control

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P1
Projects
None yet
Development

No branches or pull requests

3 participants