Issue 29603821: Noissue - Separated website-default utilities (Closed)
|
Created:
Nov. 10, 2017, 2:04 p.m. by juliandoucette Modified:
Dec. 14, 2017, 6:22 p.m. Reviewers:
ire Base URL:
https://hg.adblockplus.org/website-defaults Visibility:
Public. |
DescriptionNoissue - Separated website-default utilities
Patch Set 1 #
Total comments: 15
Patch Set 2 : Rebased and re-separated #
Total comments: 10
Patch Set 3 : Consolidated utilities #
Total comments: 5
Patch Set 4 : Addressed comments #13 #
MessagesTotal messages: 16
Please let em know what you think about the detail below. https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/_utilit... File static/scss/_utilities.scss (right): https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/_utilit... static/scss/_utilities.scss:18: * Utilities Detail: The imports below are in alphabetical order. I'd like to avoid order dependency by importing dependencies per component. e.g. if the navbar needs .container then it should import responsive-widths instead of assuming that it's available or importing all of utilities. I think that this is the logical next step if we agree to this separation.
https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/_utilit... File static/scss/_utilities.scss (right): https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/_utilit... static/scss/_utilities.scss:18: * Utilities On 2017/11/10 14:08:04, juliandoucette wrote: > Detail: The imports below are in alphabetical order. I'd like to avoid order > dependency by importing dependencies per component. e.g. if the navbar needs > .container then it should import responsive-widths instead of assuming that it's > available or importing all of utilities. I think that this is the logical next > step if we agree to this separation. If we import by component, wouldn't that mean we would have different components importing the same utilities and have duplication? https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/_utilit... static/scss/_utilities.scss:21: @import "utilities/background-colors.scss"; NIT: I think we should not have this file and have these imports in main.scss (like the form imports) https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/utiliti... File static/scss/utilities/_responsive-widths.scss (right): https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/utiliti... static/scss/utilities/_responsive-widths.scss:17: /** NIT: Change the comment style? https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/utiliti... static/scss/utilities/_responsive-widths.scss:29: .container NIT: Is the .container class a "responsive width"? https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/utiliti... File static/scss/utilities/_text-alignment.scss (right): https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/utiliti... static/scss/utilities/_text-alignment.scss:20: // Responsive NIT: I wouldn't describe this as "responsive" and "unresponsive", since those terms are typically to do with changing across screen size. I think the class names are explanatory enough, i.e. the name "start" instead of "left"
https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/_utilit... File static/scss/_utilities.scss (right): https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/_utilit... static/scss/_utilities.scss:18: * Utilities On 2017/11/13 08:33:37, ire wrote: > If we import by component, wouldn't that mean we would have different components > importing the same utilities and have duplication? I didn't think so at the time :( Apparently the fix for this is a 4.0 feature. I suspect that there is already a fix for this out there. But I'm not sure if I want to bother yet. https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/_utilit... static/scss/_utilities.scss:21: @import "utilities/background-colors.scss"; On 2017/11/13 08:33:37, ire wrote: > NIT: I think we should not have this file and have these imports in main.scss > (like the form imports) *I just thought that* - I agree. https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/utiliti... File static/scss/utilities/_responsive-widths.scss (right): https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/utiliti... static/scss/utilities/_responsive-widths.scss:17: /** On 2017/11/13 08:33:38, ire wrote: > NIT: Change the comment style? This is a JSDoc comment. It's meant to describe what the class below does; not categorize (like the other full-line comments). https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/utiliti... static/scss/utilities/_responsive-widths.scss:29: .container On 2017/11/13 08:33:38, ire wrote: > NIT: Is the .container class a "responsive width"? It was when it had responsive fixed widths. Now... sortof? It depends if you agree with the phrasing "full-width responsive" (which I just made up) meaning full-width on devices smaller than the max-width. https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/utiliti... File static/scss/utilities/_text-alignment.scss (right): https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/utiliti... static/scss/utilities/_text-alignment.scss:20: // Responsive On 2017/11/13 08:33:39, ire wrote: > NIT: I wouldn't describe this as "responsive" and "unresponsive", since those > terms are typically to do with changing across screen size. I think the class > names are explanatory enough, i.e. the name "start" instead of "left" Totally agree... Dono what I was thinking.
https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/_utilit... File static/scss/_utilities.scss (right): https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/_utilit... static/scss/_utilities.scss:18: * Utilities On 2017/11/13 12:32:59, juliandoucette wrote: > On 2017/11/13 08:33:37, ire wrote: > > If we import by component, wouldn't that mean we would have different > components > > importing the same utilities and have duplication? > > I didn't think so at the time :( > > Apparently the fix for this is a 4.0 feature. I suspect that there is already a > fix for this out there. But I'm not sure if I want to bother yet. Ack. It would be awesome to do it this way when the fix is ready. https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/utiliti... File static/scss/utilities/_responsive-widths.scss (right): https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/utiliti... static/scss/utilities/_responsive-widths.scss:17: /** On 2017/11/13 12:32:59, juliandoucette wrote: > On 2017/11/13 08:33:38, ire wrote: > > NIT: Change the comment style? > > This is a JSDoc comment. It's meant to describe what the class below does; not > categorize (like the other full-line comments). Ack. NIT: I think we typically start the comment on the first line https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/utiliti... static/scss/utilities/_responsive-widths.scss:29: .container On 2017/11/13 12:33:00, juliandoucette wrote: > On 2017/11/13 08:33:38, ire wrote: > > NIT: Is the .container class a "responsive width"? > > It was when it had responsive fixed widths. Now... sortof? > > It depends if you agree with the phrasing "full-width responsive" (which I just > made up) meaning full-width on devices smaller than the max-width. One the one hand the phrasing "full-width-responsive" makes sense literally, but because I already have a pre-existing definition of a "container", it kinda clashes in my mind. I think it's fine to leave this here because I can't think of a better place to put it.
https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/utiliti... File static/scss/utilities/_responsive-widths.scss (right): https://codereview.adblockplus.org/29603821/diff/29603822/static/scss/utiliti... static/scss/utilities/_responsive-widths.scss:17: /** On 2017/11/14 07:14:51, ire wrote: > NIT: I think we typically start the comment on the first line Not doc comments. e.g. https://hg.adblockplus.org/adblockplus/file/tip/lib/io.js#l172
Note: It makes sense to resolve https://codereview.adblockplus.org/29612659/ before uploading a new PatchSet for this.
On 2017/11/27 17:04:10, juliandoucette wrote: > Note: It makes sense to resolve https://codereview.adblockplus.org/29612659/ > before uploading a new PatchSet for this. Noted
Updated.
Thanks, a couple of comments. https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/_utilit... File static/scss/_utilities.scss (left): https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/_utilit... static/scss/_utilities.scss:1: // This file is part of website-defaults Empty file :p https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/main.scss File static/scss/main.scss (right): https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/main.sc... static/scss/main.scss:27: @import "utilities/responsive-widths.scss"; I think utilities should come earlier (after reset) because they could be used/extended by other styles https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/main.sc... static/scss/main.scss:28: @import "utilities/fixed-widths.scss"; NIT: I think some of these categories are a bit too specific, meaning we will end up with loads more files for each of these utilities when we add more. I suggest having a bit more broad category names, e.g.: - "colors" instead if background-colors - "text" instead of text-alignment" - "containers" for responsive-widths and fixed-widths (Not so sure about this one, maybe "widths") (Taking inspiration from Bootstrap's naming)
Thanks Ire! I'll update this again today. https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/_utilit... File static/scss/_utilities.scss (left): https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/_utilit... static/scss/_utilities.scss:1: // This file is part of website-defaults On 2017/12/07 11:44:20, ire wrote: > Empty file :p I'd swear that I did this right :p I blame rietveld! https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/main.scss File static/scss/main.scss (right): https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/main.sc... static/scss/main.scss:27: @import "utilities/responsive-widths.scss"; On 2017/12/07 11:44:20, ire wrote: > I think utilities should come earlier (after reset) because they could be > used/extended by other styles They ( are / can be ) extended ( in / by ) imports that are listed before them AFAIK. I will move them anyway. https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/main.sc... static/scss/main.scss:28: @import "utilities/fixed-widths.scss"; On 2017/12/07 11:44:20, ire wrote: > NIT: I think some of these categories are a bit too specific, meaning we will > end up with loads more files for each of these utilities when we add more. I > suggest having a bit more broad category names, e.g.: > > - "colors" instead if background-colors > - "text" instead of text-alignment" > - "containers" for responsive-widths and fixed-widths (Not so sure about this > one, maybe "widths") > > (Taking inspiration from Bootstrap's naming) Agreed.
https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/main.scss File static/scss/main.scss (right): https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/main.sc... static/scss/main.scss:28: @import "utilities/fixed-widths.scss"; On 2017/12/07 11:44:20, ire wrote: > NIT: I think some of these categories are a bit too specific, meaning we will > end up with loads more files for each of these utilities when we add more. I > suggest having a bit more broad category names, e.g.: > > - "colors" instead if background-colors > - "text" instead of text-alignment" > - "containers" for responsive-widths and fixed-widths (Not so sure about this > one, maybe "widths") > > (Taking inspiration from Bootstrap's naming) Done.
LGTM + NITs below https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/_utilit... File static/scss/_utilities.scss (left): https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/_utilit... static/scss/_utilities.scss:1: // This file is part of website-defaults On 2017/12/07 12:33:57, juliandoucette wrote: > On 2017/12/07 11:44:20, ire wrote: > > Empty file :p > > I'd swear that I did this right :p > > I blame rietveld! Haha! It looks like it's still here. Is it actually an issue with rietveld? (Don't forget to delete it before pushing) https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/main.scss File static/scss/main.scss (right): https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/main.sc... static/scss/main.scss:27: @import "utilities/responsive-widths.scss"; On 2017/12/07 12:33:57, juliandoucette wrote: > On 2017/12/07 11:44:20, ire wrote: > > I think utilities should come earlier (after reset) because they could be > > used/extended by other styles > > They ( are / can be ) extended ( in / by ) imports that are listed before them > AFAIK. > > I will move them anyway. You didn't move them? https://codereview.adblockplus.org/29603821/diff/29633749/static/scss/_conten... File static/scss/_content.scss (right): https://codereview.adblockplus.org/29603821/diff/29633749/static/scss/_conten... static/scss/_content.scss:94: .lead NIT: Maybe this should be under the text utilities? https://codereview.adblockplus.org/29603821/diff/29633749/static/scss/utiliti... File static/scss/utilities/_text.scss (right): https://codereview.adblockplus.org/29603821/diff/29633749/static/scss/utiliti... static/scss/utilities/_text.scss:26: .unstyled, SuperNIT: I don't think .unstyled should be under the "text" category, but I can't think of a better placement for now. Maybe something like "overrides"? We can always move it later if/when we think of something better
> It looks like it's still here. Is it actually an issue with rietveld? (Don't forget to delete it before pushing) Yup... It's Rietveld. https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/main.scss File static/scss/main.scss (right): https://codereview.adblockplus.org/29603821/diff/29631663/static/scss/main.sc... static/scss/main.scss:27: @import "utilities/responsive-widths.scss"; On 2017/12/11 15:44:03, ire wrote: > You didn't move them? oops. Done. https://codereview.adblockplus.org/29603821/diff/29633749/static/scss/_conten... File static/scss/_content.scss (right): https://codereview.adblockplus.org/29603821/diff/29633749/static/scss/_conten... static/scss/_content.scss:94: .lead On 2017/12/11 15:44:04, ire wrote: > NIT: Maybe this should be under the text utilities? Agreed. Done. https://codereview.adblockplus.org/29603821/diff/29633749/static/scss/utiliti... File static/scss/utilities/_text.scss (right): https://codereview.adblockplus.org/29603821/diff/29633749/static/scss/utiliti... static/scss/utilities/_text.scss:26: .unstyled, On 2017/12/11 15:44:04, ire wrote: > SuperNIT: I don't think .unstyled should be under the "text" category, but I > can't think of a better placement for now. Maybe something like "overrides"? We > can always move it later if/when we think of something better Hard to say. I think that unstyle will usually apply to text. But it's also tempting to create an "overrides" file to group unstyle and sr-only. I think I'll do that.
LGTM again https://codereview.adblockplus.org/29603821/diff/29633749/static/scss/utiliti... File static/scss/utilities/_text.scss (right): https://codereview.adblockplus.org/29603821/diff/29633749/static/scss/utiliti... static/scss/utilities/_text.scss:26: .unstyled, On 2017/12/14 13:37:08, juliandoucette wrote: > On 2017/12/11 15:44:04, ire wrote: > > SuperNIT: I don't think .unstyled should be under the "text" category, but I > > can't think of a better placement for now. Maybe something like "overrides"? > We > > can always move it later if/when we think of something better > > Hard to say. I think that unstyle will usually apply to text. But it's also > tempting to create an "overrides" file to group unstyle and sr-only. I think > I'll do that. +1
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
