SE7ENSKY / group-css-media-queries

CSS postprocessing: group media queries. Useful for postprocessing preprocessed CSS files.
MIT License
100 stars 19 forks source link

does this tool work? #12

Closed cyrfer closed 7 years ago

cyrfer commented 7 years ago

I tried this project and I saw no change in the output. A new file was created, but the new file still suffered from issues of the source file. I have a ton of media queries from Less transpiling to CSS that I want to group for proper selection and optimization.

Does this project actually work?

Here were my steps to test it:

ivankravchenko commented 7 years ago

Should work :) Could you provide sample of your input file?

cyrfer commented 7 years ago

Thank you for looking at the problem. I am attaching my input.

input.zip

ivankravchenko commented 7 years ago

I've run it locally with provided input, it turns this:

@media (orientation: landscape) and (min-width: 415px) {
  .col-0-landscape {
    width: 0%
  }
}

@media (orientation: landscape) and (min-width: 415px) {
  .col-offset-0-landscape {
    margin-left: 0%
  }
}

@media (orientation: landscape) and (min-width: 415px) {
  .flex-item-h-0-landscape {
    width: 0%;
    flex-basis: 0%
  }
}

into

@media (orientation: landscape) and (min-width: 415px) {
  .col-0-landscape {
    width: 0%;
  }

  .col-offset-0-landscape {
    margin-left: 0%;
  }

  .flex-item-h-0-landscape {
    width: 0%;
    flex-basis: 0%;
  }
  /* ... other rules with same media query... */

Please make sure you use latest 1.2.0 version.

cyrfer commented 7 years ago

It would be great if it did that for me. I am running on Windows 10, x64. Perhaps the implementation does not work on my OS.

I just noticed the version installed by NPM.

"group-css-media-queries": "^1.1.1",

Why does the old version get installed?

ivankravchenko commented 7 years ago

There was a minor update yesterday. Anyway, I expected this to work on Windows. I have no Windows at hand though. I will try my best to find one.

cyrfer commented 7 years ago

I updated. I still see no evidence that the tool does anything other than make a new copy of my file.

shelldandy commented 7 years ago

Can confirm this works 100% on mac, maybe its a windows thing the bug 😢

cyrfer commented 7 years ago

I did more research. I have 2 projects, each using Less 2.7.1 and the Less plugin 1.1.1 which uses this project. Both projects use the same version of all dependencies (Node 6.2.0), except for the Grunt version, and the same source .less files.

In the project that uses Grunt 0.4.5 [now 1.0.1], I see desired behavior when I use grunt less to group media queries. This means that your code can run properly on my Windows computer.

In the another project that uses Grunt 1.0.1, I see the same problems that I see when I run your code standalone, without Grunt.

Does this give you any ideas?

EDIT: I upgraded the project using Grunt 0.4.5. to use Grunt 1.1.1, and your code still works there. Something is magic about that project folder that is allowing GCssMQ to work in only that folder. Other project folders have failed to work.

UPDATE: After some time in the debugger, the AST that comes from css-parse is different in my 2 projects. In one project, there are 205 rules for my sole media query. Debugging the other project, css-parse returns with 0 rules and no media queries. I'm providing the same Less/CSS, so I cannot understand why css-parse would have different results in the same environment.

cyrfer commented 7 years ago

I FOUND THE PROBLEM The code for css-parse is completely different in my 2 projects. How did a different version of the library get installed? I tracked down when the first @media rule was parsed in the different projects.

Here is what the code should look like: image

But here is what the code looks like in the project with the problem. image

The version of css-parse bundled with GCssMQ is definitely the problem. In that version, css-parse does not add a type property and GCssMQ relies on the type property to know if the AST node is of type 'media'. Please advise how I should proceed. Thank you!

cyrfer commented 7 years ago

Please see this PR. It solved my problem. https://github.com/SE7ENSKY/group-css-media-queries/pull/14

ivankravchenko commented 7 years ago

Please confirm if it's resolved in version 1.4.0 of group-css-media-queries.

cyrfer commented 7 years ago

@ivankravchenko Confirmed! Thank you for accepting the patch.