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

Multiple of the same variants didn't applied properly on responsive styles #725

Closed
vasilenka opened this issue Aug 17, 2021 · 18 comments · Fixed by #869
Closed

Multiple of the same variants didn't applied properly on responsive styles #725

vasilenka opened this issue Aug 17, 2021 · 18 comments · Fixed by #869
Labels
bug Something isn't working P1
Projects

Comments

@vasilenka
Copy link

vasilenka commented Aug 17, 2021

Bug report

Previously I've created a discussion in https://github.com/modulz/stitches/discussions/724

Describe the bug

When 2 or more of the same variants are applied on different responsive styles breakpoints, only the last one is applied properly on the component.

To Reproduce

I've got this config:

// stitch.config.js
export const { styled } = createCss({
  ...
  media: {
    phone: `(min-width: 420px)`,
    tablet: `(min-width: 720px)`,
    desktop: `(min-width: 1024px)`,
    wide: `(min-width: 1536px)`,
  },
  ...
})

And then a Flex component that received a justify props and set it according to its variants

// Flex.js
const Flex = styled('div', {
  ...,
  variants: {
    justify: {
      start: { justifyContent: "flex-start" },
      center: { justifyContent: "center" },
      end: { justifyContent: "flex-end" },
      between: { justifyContent: "space-between" },
      evenly: { justifyContent: "space-evenly" }
    },
  },
  ...
})

// usage of Flex
return (
  <Flex justify={{ 
        "@initial": "start",
        "@phone": "center",
        "@tablet": "end",
        "@desktop": "between",
        "@wide": "end"
  }}>
   {...items}
  </Flex>
)

When I run it, the media query for @tablet never applied on the <Flex />

You can try this too:

justify={{ 
  "@initial": "start",
  "@phone": "center",
  "@tablet": "end",
  "@desktop": "end",
  "@wide": "end"
}}

And the end variant will only be applied on the @wide screen. The tablet @tablet and @desktop screen still has the center variant.

Here's a sandbox demo: https://codesandbox.io/s/stitches-responsive-problem-r43lf

System information

  • OS: [macOS]
  • Browser: tried on Chrome, Safari TP, and Firefox Nightly
  • Version of Stitches: Tried on 0.2.5 and 1.0.0-canary.10
@peduarte peduarte added this to To do in Road to 1.0 Aug 17, 2021
@peduarte
Copy link
Contributor

@jonathantneal can you verify? thanks!

@jonathantneal jonathantneal moved this from To do to In progress in Road to 1.0 Aug 17, 2021
@jonathantneal
Copy link
Contributor

I’m not certain this is a bug, but is instead a misunderstanding of media queries, and mostly because the old syntax for media queries is not very helpful to authors.

I recommend using the newer media query ranges syntax in your stitches.config.js:

import { createStitches } from "@stitches/react";

export const { styled } = createStitches({
  media: {
    phone: "(width < 720px)",
    tablet: "(720px <= width < 1024px)",
    desktop: "(1024px <= width < 1536px)",
    wide: "(1536px <= width)",
  },
});

With these breakpoints, I verified that the correct styles were applying to the correct breakpoints in the correct order.

https://codesandbox.io/s/stitches-responsive-problem-forked-ghszg?file=/src/stitches.config.js

@jonathantneal jonathantneal moved this from In progress to In review in Road to 1.0 Aug 17, 2021
@vasilenka
Copy link
Author

Sorry @jonathantneal, I tried your sandbox but it still has not applied correctly

<Flex
  justify={{
    '@initial': 'start',
    '@phone': 'center',
    '@tablet': 'end',
    '@desktop': 'between',
    '@wide': 'end',
  }}
>

The @phone and @wide has end variant on it, but the applied flex are start (which is the @initial one)

@jonathantneal
Copy link
Contributor

You are right, @vasilenka! I have been investigating this. While I don’t understand why it is happening, it has something to do with end being used twice; if that helps you with any work arounds before the fix is in.

@jonathantneal jonathantneal moved this from In review to In progress in Road to 1.0 Aug 18, 2021
@jonathantneal jonathantneal self-assigned this Aug 18, 2021
@vasilenka
Copy link
Author

@jonathantneal I made another fork of the sandbox and try to explore the problem, you can find it here: https://codesandbox.io/s/stitches-responsive-problem-forked-nz3ui
https://nz3ui.csb.app/

You may already notice it, but here're what I found:

Using the old media syntax (min-width: value)

Screen Shot 2021-08-18 at 23 02 14

This is the result when viewed on the @wide screen (>= 1536px)
The media for @tablet appeared here, but no declaration for @wide media
It's as if the logic is to take all the same variants that applied to different breakpoints, choose the most relevant rule (in this case min-width: 720px), and then applied to the latest/highest breakpoint.

I added more cases on the sandbox too, you might want to check it yourself to see the behavior. But here're some notes:

  1. Adding max-value to the range media syntax like this: (min-value < width < max-value) will yield a different result from the old syntax with only (min-width: min-value), even though the expected result is the same because the applied rules should be the same.
  2. It's even wilder if you add more duplicated variants. e.g. start, center, end, end, center will only apply the start value on every breakpoint

I hope this could help you in solving the issue

@jonathantneal jonathantneal moved this from In progress to To do in Road to 1.0 Aug 25, 2021
@jonathantneal jonathantneal removed their assignment Aug 25, 2021
@peduarte peduarte added bug Something isn't working P1 labels Aug 30, 2021
@wootsbot
Copy link

wootsbot commented Oct 1, 2021

@vasilenka @jonathantneal It worked for me by placing media at the beginning of everything

import { createStitches } from "@stitches/react";

export const { styled } = createStitches({
  media: {
    phone: "(width < 720px)",
    tablet: "(720px <= width < 1024px)",
    desktop: "(1024px <= width < 1536px)",
    wide: "(1536px <= width)",
  },
...
});

@jonathantneal
Copy link
Contributor

We haven’t had a chance to look into this much deeper. However, we can see the problem in the final output of the CSS. You will see that the shared key/value pairs under different media queries get mixed together.

--sxs {
	--sxs: 2 c-kZriBw
}

@media {
	 .c-kZriBw {
		display: flex;
		width: 100vw;
		height: 100vh;
	}
}

--sxs {
	--sxs: 3 c-kZriBw-awKDG-justify-start c-kZriBw-ceSYxj-justify-center c-kZriBw-cgrpcB-justify-between c-kZriBw-lfUpcg-justify-end
}

@media {
	 .c-kZriBw-awKDG-justify-start {
		justify-content: flex-start;
	}
	 @media (max-width: 719.9375px) {
		 .c-kZriBw-ceSYxj-justify-center {
			justify-content: center;
		}
		
	}
	 @media (min-width: 1024px) and (max-width: 1535.9375px) {
		 .c-kZriBw-cgrpcB-justify-between {
			justify-content: space-between;
		}
		
	}
	 @media (min-width: 1536px) {
		 @media (min-width: 720px) and (max-width: 1023.9375px) {
			 .c-kZriBw-lfUpcg-justify-end {
				justify-content: flex-end;
			}
			
		}
		
	}
	
}

@jkhoel
Copy link

jkhoel commented Oct 5, 2021

I have been dealing with this issue today, and I did find a couple of things that seem to matter;

  1. The sequence inside stitches.config. Here is my current setup:
export const {
  styled,
  css,
  globalCss,
  keyframes,
  getCssText,
  theme,
  createTheme,
  config,
} = createStitches({
  media: {
    // IMPORTANT! The sequence here seems to be important!
    laptops: "(1024px <= width)",
    tablets: "(768px <= width < 1024px)",
    phones: "(width < 768px)",
  },
  theme: {
 ....
  1. Assign a variant to every break-point:
<WeLoveStichCompoenent
  screenSize={{
    "@laptops": "laptop",
    "@tablets": "tablet",
    "@phones": "phone",
    "@initial": "phone",
  }}
/>

Notice that I mirror the sequence I have in my stitches.config inside the component as well. I did not have time to investigate further, but it is fair to believe that this issue gets worse with number of break-points. So for now I have limited myself to three in my case

@hadihallak
Copy link
Member

hadihallak commented Nov 8, 2021

@vasilenka would you mind validating that #869 is working as you'd expect?

@vasilenka
Copy link
Author

vasilenka commented Nov 13, 2021

@hadihallak sorry for taking so long to get back to this

I'm confused on how or which version should I test it with? there're lots of different canary version on Codesandbox

Also, I tried it on 1.2.5 and 1.0.0-canary.19 but it doesn't seems like it's working as expected yet
I already made a codesandbox project that you can use as reference for the expected result, you can see it here:
https://codesandbox.io/s/stitches-responsive-problem-forked-nz3ui?file=/src/App.js
https://nz3ui.csb.app

It's just testing the justify-content property
I used 2 different configs for the media query syntax to see if the result will be different
The label for each section is the expected result, and you can try resizing your screen to see the actual result

@hadihallak
Copy link
Member

@vasilenka No worries and thank you so much for taking a look again.
Sorry, I should've clarified that but the PR isn't currently published to npm and can only be tested on the codesandbox link in the PR
I did take a look at the link you provided and I believe that the fix is matching the behavior you described here.

Thanks again, super helpful!

@hadihallak
Copy link
Member

this is now fixed in #875 and is pending release so i'm gonna keep the issue open until we release it publicly

@hadihallak
Copy link
Member

This is now fixed in a release candidate under the version 1.2.6-0 under a canary tag.
Should be pretty stable if you'd like to upgrade now but expect a final release sometime next week once we finish migrating our products to use the updated version.

Please, don't hesitate in providing us with any feedback regarding this release. very much appreciated 🙏

@vasilenka
Copy link
Author

thank you for the hard work!
you can close this issue anytime you need to

@coreybruyere
Copy link

Thanks for this, I just want to verify my implementation as it seems to not be working using 1.2.6-0 in codesandbox: https://codesandbox.io/s/stitches-responsive-problem-forked-umghp?file=/src/App.js

@hadihallak
Copy link
Member

Hey @coreybruyere 👋
you just need to make sure that your media queries have a defined range like in this codesandbox if you know you're going to use them in a responsive variant combo where multiple media queries can match at the same time

@coreybruyere
Copy link

thanks @hadihallak - is the reasoning for this going to be documented anywhere?

@hadihallak
Copy link
Member

Published under 1.2.6 🎉

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
Development

Successfully merging a pull request may close this issue.

7 participants