search

frontendfriends / node-combine-mq

Combine matching media queries into one media query definition. Useful for CSS generated by preprocessors using nested media queries.
MIT License
19 stars 11 forks source link

fix(Task): Stabilise sort order for media queries #5

Closed kinglozzer closed 9 years ago

kinglozzer commented 9 years ago

This had me tearing my hair out. On a larger site with multiple media queries at the same width, but with different pixel densities, the results of combining the queries would be returned in a random sort order. I.e:

min-width(480px) min-width(480px) and (min-device-pixel-ratio: 2) min-width(480px) and (min-device-pixel-ratio: 3)

After combining multiple examples of these, the order of the result could be anything (meaning the first media query might override the third, rendering it useless). I eventually tracked it down to Array.prototype.sort, which was comparing the queries in a random order because it’s not a “stable” sort. After this change if the computed sortVals are equal, we fall back to sorting by their original position in the list of media queries (rather than returning 0 which results in a random order).

According to this issue, this will only affect stylesheets with more than 10 of one type of media query as the V8 engine’s sort is stable up until that number.

furzeface commented 9 years ago

Nice, thanks! Can you add an example of the media queries into https://github.com/frontendfriends/node-combine-mq/blob/master/test/actual/combined.css and duplicate it into https://github.com/frontendfriends/node-combine-mq/blob/master/test/expected/test.css - I'm in the process of writing proper tests so it'll be good to have an example of that for when I get onto it.

kinglozzer commented 9 years ago

I’ve updated the CSS files (unfortunately it adds quite a bit of bloat, necessary to hit 10 unique min-width queries). Before this change, one of the 30em queries appears in the middle of all the 30em + pixel ratio queries, after this change it appears before them where it should be.

Let me know if you want me to squash the two commits into one

furzeface commented 9 years ago

No that's fine don't worry about squashing them - I'll merge them in later on today.

I'll be splitting out the test CSS for separate working tests at some point soon so don't worry about the filesize bloat for now :smile: thanks!

kinglozzer commented 9 years ago

Thanks :smile:

furzeface commented 9 years ago

Going to do a new release of the Node package later and update https://github.com/frontendfriends/grunt-combine-mq and https://github.com/frontendfriends/gulp-combine-mq to use this feature. :+1: