Issue 29441585: No Issue - SCSS Media Query Style Proposal (Closed)
DescriptionSCSS Media Query Style Proposal
Patch Set 1 #
Total comments: 18
MessagesTotal messages: 17
Thanks Ire :) I will investigate this a little bit more before I give my opinion. I think it would be ideal if websites and UI modules aligned on the usage and style of SCSS. Will you please review this @Thomas & @Manvel?
(Please see the previous message. I meant to send it with these comments on this review.) https://codereview.adblockplus.org/29441585/diff/29441586/README.md File README.md (right): https://codereview.adblockplus.org/29441585/diff/29441586/README.md#newcode13 README.md:13: @media (max-width: $tablet-breakpoint) Note: CSS3 does support nested media queries. CSS2 does not. The CSS output from this looks like: h1 { ... } @media (...) { h1 { ... } } See https://codereview.adblockplus.org/29438582/diff/29438583/static/css/main.css for real world example. https://codereview.adblockplus.org/29441585/diff/29441586/README.md#newcode37 README.md:37: - The major issue with this method is that, by default, it ends up outputting more CSS because each media query is duplicated. For example, see the `css/default.css` file/. However, this can be resolved with a postcss plugin I found, [css-mqpacker](https://github.com/hail2u/node-css-mqpacker). This plugin consolidates media queries so we don't get duplicates, see the `css/packed.css` file. Something to note with this is that declarations will be reordered a bit since they are packed together. In my experience, this shouldn't be a problem since styles are still roughly in the same order as defined. Good find. It looks like this also works with sourcemaps (which is a requirement for my endorsement). https://codereview.adblockplus.org/29441585/diff/29441586/README.md#newcode39 README.md:39: - Another potential con with this method is potentially ending up with a larger variety of media queries. This shouldn't be much of a problem for us, as I see we already use standard breakpoints abstracted into SCSS variables to keep things consistent. Another point: See https://helloanselm.com/2014/web-performance-one-or-thousand-media-queries/ Do you know if this is still accurate? https://codereview.adblockplus.org/29441585/diff/29441586/gulpfile.js File gulpfile.js (right): https://codereview.adblockplus.org/29441585/diff/29441586/gulpfile.js#newcode1 gulpfile.js:1: var gulp = require('gulp'); Did you mean to include scss to run this? https://codereview.adblockplus.org/29441585/diff/29441586/yarn.lock File yarn.lock (right): https://codereview.adblockplus.org/29441585/diff/29441586/yarn.lock#newcode1 yarn.lock:1: # THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. What the? :D
https://codereview.adblockplus.org/29441585/diff/29441586/README.md File README.md (right): https://codereview.adblockplus.org/29441585/diff/29441586/README.md#newcode13 README.md:13: @media (max-width: $tablet-breakpoint) On 2017/05/22 11:20:09, juliandoucette wrote: > Note: CSS3 does support nested media queries. CSS2 does not. > > The CSS output from this looks like: > > h1 { > ... } > @media (...) { > h1 { > ... } } > > See > https://codereview.adblockplus.org/29438582/diff/29438583/static/css/main.css > for real world example. Noted. I wasn't actually aware that CSS3 supported nesting media queries, I only do them in SCSS in cases like this where the output is expanded. https://codereview.adblockplus.org/29441585/diff/29441586/README.md#newcode37 README.md:37: - The major issue with this method is that, by default, it ends up outputting more CSS because each media query is duplicated. For example, see the `css/default.css` file/. However, this can be resolved with a postcss plugin I found, [css-mqpacker](https://github.com/hail2u/node-css-mqpacker). This plugin consolidates media queries so we don't get duplicates, see the `css/packed.css` file. Something to note with this is that declarations will be reordered a bit since they are packed together. In my experience, this shouldn't be a problem since styles are still roughly in the same order as defined. On 2017/05/22 11:20:09, juliandoucette wrote: > Good find. It looks like this also works with sourcemaps (which is a requirement > for my endorsement). Thanks! https://codereview.adblockplus.org/29441585/diff/29441586/README.md#newcode39 README.md:39: - Another potential con with this method is potentially ending up with a larger variety of media queries. This shouldn't be much of a problem for us, as I see we already use standard breakpoints abstracted into SCSS variables to keep things consistent. On 2017/05/22 11:20:09, juliandoucette wrote: > Another point: > > See https://helloanselm.com/2014/web-performance-one-or-thousand-media-queries/ > > Do you know if this is still accurate? I haven't been able to find anything about this. My guess is that this'll only be a problem when you have a significantly large amount of MQs.
I like this approach and agree with the reasoning behind it. The only thing I'm sceptical about is the use of css-mqpacker (see comment). https://codereview.adblockplus.org/29441585/diff/29441586/README.md File README.md (right): https://codereview.adblockplus.org/29441585/diff/29441586/README.md#newcode37 README.md:37: - The major issue with this method is that, by default, it ends up outputting more CSS because each media query is duplicated. For example, see the `css/default.css` file/. However, this can be resolved with a postcss plugin I found, [css-mqpacker](https://github.com/hail2u/node-css-mqpacker). This plugin consolidates media queries so we don't get duplicates, see the `css/packed.css` file. Something to note with this is that declarations will be reordered a bit since they are packed together. In my experience, this shouldn't be a problem since styles are still roughly in the same order as defined. I can imagine that reordering CSS rules could cause unexpected behavior in case of rules that were written with their order in mind. Presumably, compression should be able to handle those duplicated media queries quite well so that we might not be able to see any significant difference in regards to the download size. Are there other reasons why you chose to group them together?
I also like the reasoning. Bit unsure about the duplication. https://codereview.adblockplus.org/29441585/diff/29441586/README.md File README.md (right): https://codereview.adblockplus.org/29441585/diff/29441586/README.md#newcode37 README.md:37: - The major issue with this method is that, by default, it ends up outputting more CSS because each media query is duplicated. For example, see the `css/default.css` file/. However, this can be resolved with a postcss plugin I found, [css-mqpacker](https://github.com/hail2u/node-css-mqpacker). This plugin consolidates media queries so we don't get duplicates, see the `css/packed.css` file. Something to note with this is that declarations will be reordered a bit since they are packed together. In my experience, this shouldn't be a problem since styles are still roughly in the same order as defined. On 2017/05/23 12:17:49, Thomas Greiner wrote: > Presumably, compression should be able to handle those duplicated media queries > quite well so that we might not be able to see any significant difference in > regards to the download size. Are there other reasons why you chose to group > them together? You mean that compression fix duplication ? Or the file size shouldn't be an issue ?
https://codereview.adblockplus.org/29441585/diff/29441586/README.md File README.md (right): https://codereview.adblockplus.org/29441585/diff/29441586/README.md#newcode37 README.md:37: - The major issue with this method is that, by default, it ends up outputting more CSS because each media query is duplicated. For example, see the `css/default.css` file/. However, this can be resolved with a postcss plugin I found, [css-mqpacker](https://github.com/hail2u/node-css-mqpacker). This plugin consolidates media queries so we don't get duplicates, see the `css/packed.css` file. Something to note with this is that declarations will be reordered a bit since they are packed together. In my experience, this shouldn't be a problem since styles are still roughly in the same order as defined. On 2017/05/23 12:17:49, Thomas Greiner wrote: > I can imagine that reordering CSS rules could cause unexpected behavior in case > of rules that were written with their order in mind. Yes there could be issues here, but since we know how the mqpacker works, we would in theory work around it in cases where we are intentionally trying to use the cascade in a particular way. > Presumably, compression should be able to handle those duplicated media queries > quite well so that we might not be able to see any significant difference in > regards to the download size. Are there other reasons why you chose to group > them together? The main reason I think it's beneficial to group them together is to avoid having hundreds of the same media query scattered across the file. Like in my example, but scaled up considerably.
https://codereview.adblockplus.org/29441585/diff/29441586/README.md File README.md (right): https://codereview.adblockplus.org/29441585/diff/29441586/README.md#newcode37 README.md:37: - The major issue with this method is that, by default, it ends up outputting more CSS because each media query is duplicated. For example, see the `css/default.css` file/. However, this can be resolved with a postcss plugin I found, [css-mqpacker](https://github.com/hail2u/node-css-mqpacker). This plugin consolidates media queries so we don't get duplicates, see the `css/packed.css` file. Something to note with this is that declarations will be reordered a bit since they are packed together. In my experience, this shouldn't be a problem since styles are still roughly in the same order as defined. On 2017/05/23 14:55:24, saroyanm wrote: > You mean that compression fix duplication ? > Or the file size shouldn't be an issue ? Since compression usually works quite well with reoccurring strings, the bandwidth used when downloading such CSS files may not be noticeably larger. We'd have to verify that assumption though. On 2017/05/23 17:20:11, ire wrote: > Yes there could be issues here, but since we know how the mqpacker works, we > would in theory work around it in cases where we are intentionally trying to use > the cascade in a particular way. It might be tricky to ensure that but fair enough. > The main reason I think it's beneficial to group them together is to avoid > having hundreds of the same media query scattered across the file. Like in my > example, but scaled up considerably. I understand that the output code won't be as nice to look at but what's the underlying issue this is trying to solve? For instance, if it could split the CSS file up into multiple smaller files (e.g. one per distinct media query) it'd allow us to download only the relevant styles.
Thanks Thomas & Manvel. Am I correct in concluding that we have agreement already excluding the issue of using or not using mqpacker? https://codereview.adblockplus.org/29441585/diff/29441586/README.md File README.md (right): https://codereview.adblockplus.org/29441585/diff/29441586/README.md#newcode13 README.md:13: @media (max-width: $tablet-breakpoint) On 2017/05/23 00:03:49, ire wrote: > On 2017/05/22 11:20:09, juliandoucette wrote: > > Note: CSS3 does support nested media queries. CSS2 does not. > > > > The CSS output from this looks like: > > > > h1 { > > ... } > > @media (...) { > > h1 { > > ... } } > > > > See > > https://codereview.adblockplus.org/29438582/diff/29438583/static/css/main.css > > for real world example. > > Noted. I wasn't actually aware that CSS3 supported nesting media queries, I only > do them in SCSS in cases like this where the output is expanded. Acknowledged. https://codereview.adblockplus.org/29441585/diff/29441586/README.md#newcode37 README.md:37: - The major issue with this method is that, by default, it ends up outputting more CSS because each media query is duplicated. For example, see the `css/default.css` file/. However, this can be resolved with a postcss plugin I found, [css-mqpacker](https://github.com/hail2u/node-css-mqpacker). This plugin consolidates media queries so we don't get duplicates, see the `css/packed.css` file. Something to note with this is that declarations will be reordered a bit since they are packed together. In my experience, this shouldn't be a problem since styles are still roughly in the same order as defined. > I can imagine that reordering CSS rules could cause unexpected behavior in case of rules that were written with their order in mind. I created a proof of concept where the order of media queries matters: https://jsfiddle.net/ume1qatj/ > It might be tricky to ensure that but fair enough. Agreed. See: https://github.com/hail2u/node-css-mqpacker#notes > I understand that the output code won't be as nice to look at but what's the underlying issue this is trying to solve? If by "this" you mean mqpacker than I believe that it is trying to solve a file size problem that may not be relevant if you use gzip (like you said, we could test this). (And I think the output is nicer to look at without mqpacker. That is why sourcemaps are necessary.) I think that the mqpacker issue is separate from the code style issue because I think it is unlikely that we will write a website/ui that contains enough individual media queries to make a significant difference. https://codereview.adblockplus.org/29441585/diff/29441586/README.md#newcode39 README.md:39: - Another potential con with this method is potentially ending up with a larger variety of media queries. This shouldn't be much of a problem for us, as I see we already use standard breakpoints abstracted into SCSS variables to keep things consistent. On 2017/05/23 00:03:49, ire wrote: > On 2017/05/22 11:20:09, juliandoucette wrote: > > Another point: > > > > See > https://helloanselm.com/2014/web-performance-one-or-thousand-media-queries/ > > > > Do you know if this is still accurate? > > I haven't been able to find anything about this. My guess is that this'll only > be a problem when you have a significantly large amount of MQs. Acknowledged.
https://codereview.adblockplus.org/29441585/diff/29441586/README.md File README.md (right): https://codereview.adblockplus.org/29441585/diff/29441586/README.md#newcode37 README.md:37: - The major issue with this method is that, by default, it ends up outputting more CSS because each media query is duplicated. For example, see the `css/default.css` file/. However, this can be resolved with a postcss plugin I found, [css-mqpacker](https://github.com/hail2u/node-css-mqpacker). This plugin consolidates media queries so we don't get duplicates, see the `css/packed.css` file. Something to note with this is that declarations will be reordered a bit since they are packed together. In my experience, this shouldn't be a problem since styles are still roughly in the same order as defined. On 2017/05/23 18:30:35, Thomas Greiner wrote: > I understand that the output code won't be as nice to look at but what's the > underlying issue this is trying to solve? > > For instance, if it could split the CSS file up into multiple smaller files > (e.g. one per distinct media query) it'd allow us to download only the relevant > styles. I think we may be talking about different things. I was saying that the benefit of using the mqpacker was to make sure that we don't end up with several of the same media query, e.g. `@media (min-width: 600px)`, each with styles for one element in them. Using the mqpacker, we can consolidate all the styles for that particular breakpoint into the one media query, while being able to write our SCSS in the way I wrote above. It seems that what you are talking about is splitting media queries into different files. This could be achieved as well. If a tool doesn't already exist to do this, we could create one. https://codereview.adblockplus.org/29441585/diff/29441586/README.md#newcode37 README.md:37: - The major issue with this method is that, by default, it ends up outputting more CSS because each media query is duplicated. For example, see the `css/default.css` file/. However, this can be resolved with a postcss plugin I found, [css-mqpacker](https://github.com/hail2u/node-css-mqpacker). This plugin consolidates media queries so we don't get duplicates, see the `css/packed.css` file. Something to note with this is that declarations will be reordered a bit since they are packed together. In my experience, this shouldn't be a problem since styles are still roughly in the same order as defined. On 2017/05/24 11:44:18, juliandoucette wrote: > I created a proof of concept where the order of media queries matters: > https://jsfiddle.net/ume1qatj/ Yes I totally agree that there are cases where order will matter. As a side note, I tend to stick to only one direction of media query (typically, min-width). Additionally, I would also suggest always writing them in a logical order, e.g. start with smallest and increase the size. This would solve issues of the order, but of course requires a more strictly enforced style of writing CSS. > If by "this" you mean mqpacker than I believe that it is trying to solve a file > size problem that may not be relevant if you use gzip (like you said, we could > test this). (And I think the output is nicer to look at without mqpacker. That > is why sourcemaps are necessary.) > > I think that the mqpacker issue is separate from the code style issue because I > think it is unlikely that we will write a website/ui that contains enough > individual media queries to make a significant difference. Noted. I was (maybe wrongly) working under the assumption that having less media queries (especially ones that in theory could be consolidated) would be noticeably better than the alternative. So we should definitely do some tests ourselves to find out, and we can decide if the mqpacker is even necessary.
On 2017/05/24 11:44:18, juliandoucette wrote: > Am I correct in concluding that we have agreement already excluding the issue of > using or not using mqpacker? I think so. On 2017/05/24 13:06:27, ire wrote: > I think we may be talking about different things. I was saying that the benefit > of using the mqpacker was to make sure that we don't end up with several of the > same media query, e.g. `@media (min-width: 600px)`, each with styles for one > element in them. Using the mqpacker, we can consolidate all the styles for that > particular breakpoint into the one media query, while being able to write our > SCSS in the way I wrote above. > > It seems that what you are talking about is splitting media queries into > different files. This could be achieved as well. If a tool doesn't already exist > to do this, we could create one. No worries, we're fine. I was just mentioning it as an example so sorry about the confusion. > So we should definitely do some tests > ourselves to find out, and we can decide if the mqpacker is even necessary. Sounds good.
LGTM Please open a separate issue for the mqpacker proposal. https://codereview.adblockplus.org/29441585/diff/29441586/README.md File README.md (right): https://codereview.adblockplus.org/29441585/diff/29441586/README.md#newcode37 README.md:37: - The major issue with this method is that, by default, it ends up outputting more CSS because each media query is duplicated. For example, see the `css/default.css` file/. However, this can be resolved with a postcss plugin I found, [css-mqpacker](https://github.com/hail2u/node-css-mqpacker). This plugin consolidates media queries so we don't get duplicates, see the `css/packed.css` file. Something to note with this is that declarations will be reordered a bit since they are packed together. In my experience, this shouldn't be a problem since styles are still roughly in the same order as defined. > Noted. I was (maybe wrongly) working under the assumption that having less media > queries (especially ones that in theory could be consolidated) would be > noticeably better than the alternative. So we should definitely do some tests > ourselves to find out, and we can decide if the mqpacker is even necessary. I would like to discuss mqpacker in a separate issue. (If I understand correctly, it will not make a significant difference.)
On 2017/05/30 10:10:48, juliandoucette wrote: > Please open a separate issue for the mqpacker proposal. Okay
@Thomas can you please LGTM or not for UI module so that we can close this.
On 2017/07/14 12:16:43, juliandoucette wrote: > @Thomas can you please LGTM or not for UI module so that we can close this. Sorry, wasn't aware that this was still open. LGTM and I agree with Julian's recommendation on separating the mqpacker proposal.
On 2017/07/14 12:19:18, Thomas Greiner wrote: Thanks Thomas :) Can you please close this Ire?
On 2017/07/25 13:49:51, juliandoucette wrote: > Can you please close this Ire? Done. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
