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

Incorrect media query concats #512

Closed
romainmenke opened this issue Dec 1, 2022 · 8 comments · Fixed by #513
Closed

Incorrect media query concats #512

romainmenke opened this issue Dec 1, 2022 · 8 comments · Fixed by #513
Labels

Comments

@romainmenke
Copy link
Collaborator

romainmenke commented Dec 1, 2022

@import "ignore.css" (min-width: 25em);
/* ignore.css */
@import "http://css-screen" screen;

Becomes :

@import "http://css-screen" (min-width: 25em) and screen;

Should be :

@import "http://css-screen" screen and (min-width: 25em);

It might be interesting to adopt our media query parser : https://github.com/csstools/postcss-plugins/tree/postcss-preset-env--v8/packages/media-query-list-parser#readme

It has a typed object model which makes it easier to avoid these issues.


Alternatively it is possible to "special case" these keywords :

  • screen
  • print
  • all
  • not

And then sorting the media query lists :

  1. these keywords first
  2. everything else later
  3. join with and
romainmenke added a commit to romainmenke/postcss-import that referenced this issue Dec 1, 2022
@RyanZim
Copy link
Collaborator

RyanZim commented Dec 1, 2022

Sorry, my CSS knowledge isn't good enough here, why does order matter here?

@romainmenke
Copy link
Collaborator Author

https://w3c.github.io/csswg-drafts/mediaqueries/#mq-syntax

The syntax description is fairly complex but as a rule things like screen can not appear after (min-width: 300px)

simplified : <modifier> <type> and <condition> and <condition>

valid :

@media screen {};
@media only screen {}
@media screen and (min-width: 300px) {}
@media not screen {}
@media not screen and (min-width : 300px) {}
@media screen and (min-width: 300px) and (min-height: 300px) {}

Not valid :

@media (min-width: 300px) and screen {}
@media (min-width: 300px) and not screen {}
...

@RyanZim
Copy link
Collaborator

RyanZim commented Dec 1, 2022

It might be interesting to adopt our media query parser : https://github.com/csstools/postcss-plugins/tree/postcss-preset-env--v8/packages/media-query-list-parser#readme

How complicated would this be?

@romainmenke
Copy link
Collaborator Author

How complicated would this be?

Very, and given that no one has ever opened an issue about this I don't know if it is worth it.

@RyanZim
Copy link
Collaborator

RyanZim commented Dec 1, 2022

In that case, I think I'll merge your regex hack and see if anyone complains 😉

@RyanZim RyanZim added the bug label Dec 1, 2022
@romainmenke
Copy link
Collaborator Author

Thank you for the reviews and the release 🙇

@RyanZim
Copy link
Collaborator

RyanZim commented Dec 2, 2022

Thanks for all your contributions. BTW, you're now collaborator here; I'll still have to do releases, and we should probably still do PRs mostly, but you do have the merge bit.

@romainmenke
Copy link
Collaborator Author

Thank you for trusting me with this package 🎉

Yes I also prefer to do PR's, it so valuable to have another opinion on changes :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants