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

Sort media queries #24

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@Maks3w
Contributor

Maks3w commented Oct 22, 2014

Note: Function code from https://github.com/buildingblocks/grunt-combine-media-queries

  • Why this feature should be merged even breaking the backward compatibility?

Following the idea there is not "unsafe group" only bad CSS code (#16 (comment)) I have ported this sort algorithm from https://github.com/buildingblocks/grunt-combine-media-queries

  • Where MQpacker fail and really is a fail in the library?

MQPacker fail because "have" a first win algoritm. So the result CSS has the same order of media queries as found in the original source.

But what happens in the following scenario?

/* From WidgetA.less */
.widgetA: { foo: 'xxx' }
@media (min-width:1024px) {
 .widgetA: {'foo': 'yyy'}
}

/* From WidgetB.less */
.widgetB: { foo: 'xxx' }
@media (min-width:480px) {
 .widgetB: {'foo': 'yyy'}
}
@media (min-width:1024px) {
 .widgetB: {'foo': 'zzz'}
}

Both widgets are well written CSS individually and have the media queries sorted.
But after process the CSS we have the following result:

.widgetA: { foo: 'xxx' }
.widgetB: { foo: 'xxx' }
@media (min-width:1024px) {
 .widgetA: {'foo': 'yyy'}
 .widgetB: {'foo': 'zzz'}   // OOOps this now have lower priority!!!.
}
@media (min-width:480px) {
 .widgetB: {'foo': 'yyy'}
}

So this PR fix this situation enforcing to use a specific sorted algoritm. CSS programers MUST follow this algoritm if they want to have a working CSS file after use MQpacker

The result is:

.widgetA: { foo: 'xxx' }
.widgetB: { foo: 'xxx' }
@media (min-width:480px) {
 .widgetB: {'foo': 'yyy'}
}
@media (min-width:1024px) {
 .widgetA: {'foo': 'yyy'}
 .widgetB: {'foo': 'zzz'}
}

The algoritm used here is the same used at https://github.com/buildingblocks/grunt-combine-media-queries (MIT license)

These are the rules applied:

  1. Rules without media queries.
  2. min-width rules sorted in ascending order by width value.
  3. print media and min-width rules sorted in ascending order by width value.
  4. min-height rules sorted in ascending order by height value.
  5. print media and min-height rules sorted in ascending order by height value.
  6. max-width rules sorted in descending order by width value.
  7. print media and max-width rules sorted in descending order by width value.
  8. max-height rules sorted in descending order by height value.
  9. print media and max-height rules sorted in descending order by height value.
  10. Rest of print media queries
@hail2u

This comment has been minimized.

Show comment
Hide comment
@hail2u

hail2u Oct 22, 2014

Owner

This algorithm works fine on many situations. However, I don't think this is a good solution.

  • What if same features have different unit?
  • What if different queries are separated by comma?
  • What if queries begin with not keywords?

Implementing algorithm for sorting queries can be very hard. I don't think it can be done by handling only (min|max)-(width|height) features.

And this would not work fine on forthcoming spec changes.


Also, I usually resolve these type of problem by placing dummy media queries on top of CSS (in other words, defining important queries order at first):

@media (min-width: 480px) { /!* 480px+ */ }
@media (min-width: 1024px) { /!* 1024px+ */ }

@import "WidgetA.less";
@import "WidgetB.less";
Owner

hail2u commented Oct 22, 2014

This algorithm works fine on many situations. However, I don't think this is a good solution.

  • What if same features have different unit?
  • What if different queries are separated by comma?
  • What if queries begin with not keywords?

Implementing algorithm for sorting queries can be very hard. I don't think it can be done by handling only (min|max)-(width|height) features.

And this would not work fine on forthcoming spec changes.


Also, I usually resolve these type of problem by placing dummy media queries on top of CSS (in other words, defining important queries order at first):

@media (min-width: 480px) { /!* 480px+ */ }
@media (min-width: 1024px) { /!* 1024px+ */ }

@import "WidgetA.less";
@import "WidgetB.less";
@Maks3w

This comment has been minimized.

Show comment
Hide comment
@Maks3w

Maks3w Oct 23, 2014

Contributor

May is not the "solution" for all neither the best solution. But at least as you said it's a solution for many cases and does not make worst the rest of the cases.

Feel free to create a pair of fixtures about how the rest of media queries should be managed and then we can start to look in way of do that.

Forhcoming changes it's the future. Try to fix the issues from today and fix tomorrow the issues of tomorrow.

Your way of put code outside of the "less" files in the main file does not follow the "encapsulate" behavior expected from a design by component pattern.

This proposal has in mind not only what code you write but also the code from third party "well written" libraries like Bootstrap. So you can end with a packed version of the libraries and your code.

Contributor

Maks3w commented Oct 23, 2014

May is not the "solution" for all neither the best solution. But at least as you said it's a solution for many cases and does not make worst the rest of the cases.

Feel free to create a pair of fixtures about how the rest of media queries should be managed and then we can start to look in way of do that.

Forhcoming changes it's the future. Try to fix the issues from today and fix tomorrow the issues of tomorrow.

Your way of put code outside of the "less" files in the main file does not follow the "encapsulate" behavior expected from a design by component pattern.

This proposal has in mind not only what code you write but also the code from third party "well written" libraries like Bootstrap. So you can end with a packed version of the libraries and your code.

@Maks3w

This comment has been minimized.

Show comment
Hide comment
@Maks3w

Maks3w Oct 23, 2014

Contributor

Also. The sort algorithm is needed since there is no know how the media queries will be sorted next time.

If you add or modify some file in the project (which is the first to be processed) and only use the most highest width media query the result CSS file will have the most highest width media query first. (The scenario described in the PR)

Contributor

Maks3w commented Oct 23, 2014

Also. The sort algorithm is needed since there is no know how the media queries will be sorted next time.

If you add or modify some file in the project (which is the first to be processed) and only use the most highest width media query the result CSS file will have the most highest width media query first. (The scenario described in the PR)

@Maks3w Maks3w referenced this pull request Oct 24, 2014

Closed

Unsafe grouping #16

@hail2u

This comment has been minimized.

Show comment
Hide comment
@hail2u

hail2u Oct 24, 2014

Owner

it's a solution for many cases and

It's a bug for many cases also. How to sort min-width: 320px and min-width: 20em? Or how to sort max-aspect-ratio: 1/2 and max-aspect-ratio: 16/9? I don't think units in queries can be sorted. When we try to support only current css3-mediaqueries, a sorting algorithm can be very gigantic.

This is the reason why I stopped using grunt-combine-media-queries and create this library.

This proposal has in mind not only what code you write but also the code from third party "well written" libraries like Bootstrap.

CSS libraries should (I think must) remain untouched. Optimizing CSS libraries' code causes unexpected behavior to both developers and users. Developers optimize CSS code that only they write.

Owner

hail2u commented Oct 24, 2014

it's a solution for many cases and

It's a bug for many cases also. How to sort min-width: 320px and min-width: 20em? Or how to sort max-aspect-ratio: 1/2 and max-aspect-ratio: 16/9? I don't think units in queries can be sorted. When we try to support only current css3-mediaqueries, a sorting algorithm can be very gigantic.

This is the reason why I stopped using grunt-combine-media-queries and create this library.

This proposal has in mind not only what code you write but also the code from third party "well written" libraries like Bootstrap.

CSS libraries should (I think must) remain untouched. Optimizing CSS libraries' code causes unexpected behavior to both developers and users. Developers optimize CSS code that only they write.

@Maks3w Maks3w referenced this pull request Feb 25, 2015

Merged

[WIP] Sort `min-width` queries #28

17 of 17 tasks complete
@hail2u

This comment has been minimized.

Show comment
Hide comment
@hail2u

hail2u Feb 28, 2015

Owner

I want to reject this pull request for the following reasons:

  1. Sorting order is very clear, but it doesn't explain why
  2. It uses Object.keys() that not guarantee order especially in v8 engine
  3. I don't want to maintain a code “borrowed” by others
  4. It's too big compared with main code
Owner

hail2u commented Feb 28, 2015

I want to reject this pull request for the following reasons:

  1. Sorting order is very clear, but it doesn't explain why
  2. It uses Object.keys() that not guarantee order especially in v8 engine
  3. I don't want to maintain a code “borrowed” by others
  4. It's too big compared with main code
@Maks3w

This comment has been minimized.

Show comment
Hide comment
@Maks3w

Maks3w Feb 28, 2015

Contributor

Sorting order puts first the value less specific. So the minimum min-width cover is less specific than maximum min-width and viceversa with max-width (maximum is less specific than the minimum)

It's the same as put general/root rules (less specific) before media queries (more specific)

Contributor

Maks3w commented Feb 28, 2015

Sorting order puts first the value less specific. So the minimum min-width cover is less specific than maximum min-width and viceversa with max-width (maximum is less specific than the minimum)

It's the same as put general/root rules (less specific) before media queries (more specific)

@hail2u

This comment has been minimized.

Show comment
Hide comment
@hail2u

hail2u Mar 9, 2015

Owner

I've just released v3.1.0. This release supports sort option for sorting only min-width queries and custom sorting function. Probably, you can use this grunt-combine-media-queries method with this option.

Owner

hail2u commented Mar 9, 2015

I've just released v3.1.0. This release supports sort option for sorting only min-width queries and custom sorting function. Probably, you can use this grunt-combine-media-queries method with this option.

@hail2u hail2u closed this Mar 9, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment