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

Responsive variant order is applied incorrectly #885

Open
hwhmeikle opened this issue Dec 3, 2021 · 10 comments
Open

Responsive variant order is applied incorrectly #885

hwhmeikle opened this issue Dec 3, 2021 · 10 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@hwhmeikle
Copy link

hwhmeikle commented Dec 3, 2021

Bug report

Describe the bug

I have a Stack component that is using the responsive variant syntax to define an @initial value, a @md, and a @lg value for its gap variant. At the @lg breakpoint, the @md value is being applied instead.

It looks like it is being caused by the fact that a parent Stack component is using the same breakpoint -> value variant. In my example, I have a parent Stack that has { '@initial': 8, '@lg': 20 } for its gap variant, and a descendent Stack with { '@initial': 8, '@md': 10, '@lg': 20 } for its gap variant. The @lg values seems to be clashing. You can verify this by changing the parent Stack's @lg value to something other than 20, or by removing it completely, and the variants are applied in the correct order.

I imagine what is happening is that the @md class is generated and injected after the @lg class, and the cascade does the rest.

To Reproduce

Code Sandbox reproduction

Expected behavior

Responsive variants are applied in the correct order.

Screenshots

Here's the variants being incorrectly applied:

Screenshot of variants classes applied in the wrong order

System information

  • Version of Stitches: 1.2.6
@hadihallak
Copy link
Member

@hwhmeikle Hey 👋

Styles are generally injected by the order of rendering (with a few exceptions that we’ve introduced recently) so for this case, when multiple variant values might show up on different breakpoints, our recommended approach is to make use of ranged media queries so that the different breakpoints aren’t fighting with each others based on the order that they are injected in.

Please see this modified codesandbox and let me know if it matches the output you would expect.

I’ve modified stitches.config.ts to include a range version of the breakpoints and then used it in App.tsx on line 26

@hwhmeikle
Copy link
Author

hwhmeikle commented Dec 3, 2021

Hey, thanks for the quick reply!

That definitely seems to solve this scenario. This might be a contrived example, but what would you suggest if we were skipping a breakpoint e.g. '@initial': 8, '@rmd': 15, '@xl:': 30}? Because @rmd only ranges between md/lg, at the lg breakpoint it reverts to what was defined at @initial. I'm leaning towards '@initial': 8, '@rmd': 15, '@rlg': 15, '@xl:': 30}.

Perhaps that's just part of the discipline of using ranged media queries inside your responsive variants, you have to ensure you're defining a value at every breakpoint between your base/highest variant.

Code sandbox of the above scenario

@hadihallak
Copy link
Member

Yes, what you suggested is probably how I would do it.

We're also thinking about ordering the responsive variants styles by the order of breakpoints in the media object so that your original code would work without defining ranged media queries since @lg would always be injected after @md no matter what but we still haven't fully decided on that yet.

@csantos1113
Copy link
Contributor

csantos1113 commented Dec 3, 2021

Hey all, jumping into this conversation.
While I understand what @hadihallak is saying/suggesting I think this is such a weird way to solve the issue :(. in @hwhmeikle example is pretty simple and shows the use case, but you can perfectly have a huge page with many Stack components rendered in many different places with many different variants.
So a random <Stack gap={{ "@initial": 8, "@lg": 20 }} > somewhere in the page could affect badly another random <Stack gap={{ "@initial": 8, "@md": 10, "@lg": 20 }}> somewhere else in the page 😢👎.

I also understand the suggested solution of having range medias to target specific medias, but that is not really the solution because the "bad" css selector is already injected in the DOM in the order it was injected, so another <Stack component could be instantiated elsewhere with the "wrong" responsive variants too.

Basically what I'm saying is that an instance of Stack component must not care about what other Stack instances might have done with their styles.

To finish up on the range medias approach: yes, you could use range medias everywhere, and that would solve the problem, but that is not how you usually write css/styles. It'll be pretty annoying having to specify the styles for all medias all the time (because you can't really use normal medias, ex: (min-width: 740px) because then you might be the one injecting the "bad" css selector to the DOM)


Ok, so, what could we do?

I haven't looked at the internals of stitches, but perhaps, instead of creating css classNames for each component + variant + value combination, we could try to create unique css classNames for the whole variant per se.

So for example, say this is the prop gap={{ "@initial": 8, "@md": 10, "@lg": 20 }},
it currently generates 3 classNames:

c-dhzjXW-ctMUrj-gap-8 // with its media query selector
c-dhzjXW-cJJBop-gap-10 // with its media query selector
c-dhzjXW-deging-gap-20 // with its media query selector

We could generate a unique className for the group, example c-dhzjXW-ctMUrj-gap-cccppp that has the 3 mediaqueries inside.
Then some sort of memoization strategy is applied and if any other Stack instance uses the same exact props then it reuses that className, otherwise generate a new className for the new group, and so on and so on.

Performance?
This is maybe? less performant than what stitches has right now, but again, a component instance styles shouldn't care what other component instance styles have; nor we should be using range media queries

@hwhmeikle
Copy link
Author

@hadihallak I have to agree with @csantos1113 that defining ranged media queries everywhere isn't the best developer experience. I like your solution around ordering the responsive variants by how they're defined in the media object.

@hadihallak
Copy link
Member

@csantos1113 @hwhmeikle

Basically what I'm saying is that an instance of Stack component must not care about what other Stack instances might have done with their styles.

I 100% agree with that statement — Rendering order should never produce different visual results.
The solution I suggested is in no way the final form and the recent release to stitches mainly focus on fixing and improving on some of the most pressing injection order issues.

we've also experimented with the idea of generating unique hashes but as you mentioned, this is likely to cause performance implications so we're not set on that yet.

All in all, I do think that this is a bug, and the recommended approach for now is to use ranged media queries as a workaround until we provide a fix for it which is likely land once we figure out React 18 and concurrency support.
Until then, we're gonna keep this issue open.

@peduarte peduarte added the bug Something isn't working label Dec 6, 2021
@lucasconstantino
Copy link

lucasconstantino commented Dec 15, 2021

I've hit the same issue when developing a grid-system. The problem occurs when a combination of media/variant is used a second time – as the combo was found before, its rendering is market as cached, and therefore it isn't rendered again. Not a problem generally, but a problem if there are other media selectors on the same component which would match as well and weren't rendered before: then, the last found is rendered last, and gains specificity.

Solutions

No automatic solution is possible, as this is how CSS works. If you are doing mobile-first, this might be a pain and seems possible to fix, but you could as well be using other kinds of media queries or even top-to-bottom media selectors, and therefore Stitches cannot assume it.

1) Configuration:

I would suggest stitches renders media styles in split up style tags – following whatever order is found on the createStitches.media.

We should advise users that numeric keys could break this, for the well know order of numeric props issues on JavaScript.

2) Workaround:

Avoid using variants, and use local variables instead:

// From
const Component = styled('div', { variants: { color: { blue: { color: 'blue' }, red: { color: 'red' } } } })
<Component color={ { '@bp1': 'blue', '@bp2': 'red' } } />

// To
const Component = styled('div', { $$color: 'initial', color: '$$color' })
<Component css={ { '@bp1': { $$color: 'blue' }, '@bp2': { color: "red' } } } />

3) Dynamic styled-components:

Create a wrapper component that generates one new styled-component per media/variant combos found. This is probably not performant, but could serve as a workaround.


Here is a codesanbox example for 2) and 3).

@csantos-nydig
Copy link

stitches folks, are there any updates on this?

maintainers are so quiet lately. Started to worry about this lib not longer being maintained? 😟

@hadihallak
Copy link
Member

@csantos-nydig not currently this is going to be fixed but not as a minor update as it requires some decent re-work of the internals and rigorous testing before this is fixed. so, rest assured that Stitches is maintained and we're working on some exciting things that we hope to share very soon

@hadihallak hadihallak added this to the 1.3.0 milestone Jun 22, 2022
@hadihallak hadihallak self-assigned this Jun 22, 2022
@Yanick64
Copy link

Hey @hadihallak,

Is there a status update? Unfortunately, we are now running into exactly the same problem.

Thanks!

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

No branches or pull requests

7 participants