Issue 29529767: Issue 5573 - Add a CSS reset to website-defaults (Closed)
|
Created:
Aug. 28, 2017, 5:25 p.m. by ire Modified:
Sept. 6, 2017, 10:59 p.m. Reviewers:
juliandoucette Base URL:
https://hg.adblockplus.org/website-defaults Visibility:
Public. |
DescriptionIssue 5573 - Add a CSS reset to website-defaults
Patch Set 1 #
Total comments: 13
Patch Set 2 : Reorder selectors, move reset styles from base to reset.scss #
Total comments: 12
Patch Set 3 : Address NITs #
MessagesTotal messages: 10
First attempt ready for review
https://codereview.adblockplus.org/29529767/diff/29529768/scss/_reset.scss File scss/_reset.scss (right): https://codereview.adblockplus.org/29529767/diff/29529768/scss/_reset.scss#ne... scss/_reset.scss:20: * CSS Reset I created a different file instead of adding this to _base.scss . I think some of the styles from there should be moved here (mostly the styles of elements not classes) https://codereview.adblockplus.org/29529767/diff/29529768/scss/_reset.scss#ne... scss/_reset.scss:23: /* Reset margins, paddings, and font globally */ Most of this is based on Eric Meyer's reset (http://meyerweb.com/eric/tools/css/reset/). It's in the public domain which is why I didn't add any attribution. https://codereview.adblockplus.org/29529767/diff/29529768/scss/_reset.scss#ne... scss/_reset.scss:25: // General This was my attempt to organise the selectors better. I don't know if the comments are helpful? https://codereview.adblockplus.org/29529767/diff/29529768/scss/_reset.scss#ne... scss/_reset.scss:70: /* Remove list styles */ I'm undecided if we want/need this
Did you base this reset on any others in specific?
On 2017/08/28 18:39:20, juliandoucette wrote: > Did you base this reset on any others in specific? Yup, I mentioned in [this comment](https://codereview.adblockplus.org/29529767/diff/29529768/scss/_reset.scss#newcode23) this is based on Eric Meyer's reset
LGTM + NITs (See comments) https://codereview.adblockplus.org/29529767/diff/29529768/scss/_reset.scss File scss/_reset.scss (right): https://codereview.adblockplus.org/29529767/diff/29529768/scss/_reset.scss#ne... scss/_reset.scss:20: * CSS Reset On 2017/08/28 17:29:10, ire wrote: > I created a different file instead of adding this to _base.scss . I think some > of the styles from there should be moved here (mostly the styles of elements not > classes) Acknowledged. (I don't mind if you do that in this issue or another) https://codereview.adblockplus.org/29529767/diff/29529768/scss/_reset.scss#ne... scss/_reset.scss:23: /* Reset margins, paddings, and font globally */ On 2017/08/28 17:29:10, ire wrote: > Most of this is based on Eric Meyer's reset > (http://meyerweb.com/eric/tools/css/reset/). It's in the public domain which is > why I didn't add any attribution. I think that we should mention it in our README anyway. https://codereview.adblockplus.org/29529767/diff/29529768/scss/_reset.scss#ne... scss/_reset.scss:25: // General On 2017/08/28 17:29:10, ire wrote: > This was my attempt to organise the selectors better. I don't know if the > comments are helpful? I like them. https://codereview.adblockplus.org/29529767/diff/29529768/scss/_reset.scss#ne... scss/_reset.scss:60: del, dfn, ins, kbd, samp, var, output, ruby NIT: [del, ins, strike, mark] seem related (editing) and [code, pre] + arguably [samp, var, output, ruby] seem related. + kbd, samp are also quite similar. I don't mind if you leave this as-is (it's only important that they are included). But I would consider grouping the items mentioned above differently. https://codereview.adblockplus.org/29529767/diff/29529768/scss/_reset.scss#ne... scss/_reset.scss:70: /* Remove list styles */ On 2017/08/28 17:29:10, ire wrote: > I'm undecided if we want/need this Acknowledged.
Made some updates. I'll let you have a final look before I push https://codereview.adblockplus.org/29529767/diff/29529768/scss/_reset.scss File scss/_reset.scss (right): https://codereview.adblockplus.org/29529767/diff/29529768/scss/_reset.scss#ne... scss/_reset.scss:20: * CSS Reset On 2017/08/30 10:04:28, juliandoucette wrote: > On 2017/08/28 17:29:10, ire wrote: > > I created a different file instead of adding this to _base.scss . I think some > > of the styles from there should be moved here (mostly the styles of elements > not > > classes) > > Acknowledged. > > (I don't mind if you do that in this issue or another) Done. https://codereview.adblockplus.org/29529767/diff/29529768/scss/_reset.scss#ne... scss/_reset.scss:23: /* Reset margins, paddings, and font globally */ On 2017/08/30 10:04:28, juliandoucette wrote: > On 2017/08/28 17:29:10, ire wrote: > > Most of this is based on Eric Meyer's reset > > (http://meyerweb.com/eric/tools/css/reset/). It's in the public domain which > is > > why I didn't add any attribution. > > I think that we should mention it in our README anyway. Done. https://codereview.adblockplus.org/29529767/diff/29529768/scss/_reset.scss#ne... scss/_reset.scss:25: // General On 2017/08/30 10:04:28, juliandoucette wrote: > On 2017/08/28 17:29:10, ire wrote: > > This was my attempt to organise the selectors better. I don't know if the > > comments are helpful? > > I like them. Great https://codereview.adblockplus.org/29529767/diff/29529768/scss/_reset.scss#ne... scss/_reset.scss:60: del, dfn, ins, kbd, samp, var, output, ruby On 2017/08/30 10:04:28, juliandoucette wrote: > NIT: [del, ins, strike, mark] seem related (editing) and [code, pre] + arguably > [samp, var, output, ruby] seem related. + kbd, samp are also quite similar. > > I don't mind if you leave this as-is (it's only important that they are > included). But I would consider grouping the items mentioned above differently. Done. I moved them to typography and ordered them together by line
https://codereview.adblockplus.org/29529767/diff/29533555/gulpfile.js File gulpfile.js (left): https://codereview.adblockplus.org/29529767/diff/29533555/gulpfile.js#oldcode29 gulpfile.js:29: gulp.task("watch", function () Merge or rebase gone wrong? https://codereview.adblockplus.org/29529767/diff/29533555/scss/_reset.scss File scss/_reset.scss (right): https://codereview.adblockplus.org/29529767/diff/29533555/scss/_reset.scss#ne... scss/_reset.scss:29: h1, h2, h3, h4, h5, h6, NIT: I like how these are grouped, but it is inconsistent with the selectors below. If our code style forces us to list each selector on a new line then I propose that we propose conditions in which we can group them on the same line e.g. I think blockquote, q should be able to go on the same line (near the bottom of this file); at least. https://codereview.adblockplus.org/29529767/diff/29533555/scss/_reset.scss#ne... scss/_reset.scss:96: /* Remove the margin in all browsers (opinionated). */ NIT: I would prefer to order these in terms of specificity e.g. *, html, body, ... https://codereview.adblockplus.org/29529767/diff/29533555/scss/_reset.scss#ne... scss/_reset.scss:113: blockquote, NIT: We could point out which browsers this is for https://codereview.adblockplus.org/29529767/diff/29533555/scss/_reset.scss#ne... scss/_reset.scss:119: blockquote:before, NIT: We could point out which browsers this is for
https://codereview.adblockplus.org/29529767/diff/29533555/gulpfile.js File gulpfile.js (left): https://codereview.adblockplus.org/29529767/diff/29533555/gulpfile.js#oldcode29 gulpfile.js:29: gulp.task("watch", function () On 2017/09/01 10:30:58, juliandoucette wrote: > Merge or rebase gone wrong? Yup, fixed. https://codereview.adblockplus.org/29529767/diff/29533555/scss/_reset.scss File scss/_reset.scss (right): https://codereview.adblockplus.org/29529767/diff/29533555/scss/_reset.scss#ne... scss/_reset.scss:29: h1, h2, h3, h4, h5, h6, On 2017/09/01 10:30:58, juliandoucette wrote: > NIT: I like how these are grouped, but it is inconsistent with the selectors > below. If our code style forces us to list each selector on a new line then I > propose that we propose conditions in which we can group them on the same line > e.g. I think blockquote, q should be able to go on the same line (near the > bottom of this file); at least. I considered this particular selector (since it's so large) to be the exception. Perhaps the condition in which they can be grouped together is if the number of selectors is greater than a specific number (maybe 10?) For the other selectors like `blockquote, q`, I don't really see the argument for them also being changed given the code style is that selectors should be on a new line other than the existence of this mega selector. https://codereview.adblockplus.org/29529767/diff/29533555/scss/_reset.scss#ne... scss/_reset.scss:96: /* Remove the margin in all browsers (opinionated). */ On 2017/09/01 10:30:59, juliandoucette wrote: > NIT: I would prefer to order these in terms of specificity e.g. *, html, body, > ... I think it makes more sense in terms of readability to have box-sizing: border-box to come before box-sizing: inherit, particularly since the styles are grouped by purpose, but I don't feel particularly strongly about it so changed. https://codereview.adblockplus.org/29529767/diff/29533555/scss/_reset.scss#ne... scss/_reset.scss:113: blockquote, On 2017/09/01 10:30:58, juliandoucette wrote: > NIT: We could point out which browsers this is for Done. https://codereview.adblockplus.org/29529767/diff/29533555/scss/_reset.scss#ne... scss/_reset.scss:119: blockquote:before, On 2017/09/01 10:30:59, juliandoucette wrote: > NIT: We could point out which browsers this is for Done.
LGTM https://codereview.adblockplus.org/29529767/diff/29533555/scss/_reset.scss File scss/_reset.scss (right): https://codereview.adblockplus.org/29529767/diff/29533555/scss/_reset.scss#ne... scss/_reset.scss:29: h1, h2, h3, h4, h5, h6, On 2017/09/05 06:55:52, ire wrote: > On 2017/09/01 10:30:58, juliandoucette wrote: > > NIT: I like how these are grouped, but it is inconsistent with the selectors > > below. If our code style forces us to list each selector on a new line then I > > propose that we propose conditions in which we can group them on the same line > > e.g. I think blockquote, q should be able to go on the same line (near the > > bottom of this file); at least. > > I considered this particular selector (since it's so large) to be the exception. > Perhaps the condition in which they can be grouped together is if the number of > selectors is greater than a specific number (maybe 10?) > > For the other selectors like `blockquote, q`, I don't really see the argument > for them also being changed given the code style is that selectors should be on > a new line other than the existence of this mega selector. Acknowledged. https://codereview.adblockplus.org/29529767/diff/29533555/scss/_reset.scss#ne... scss/_reset.scss:96: /* Remove the margin in all browsers (opinionated). */ On 2017/09/05 06:55:51, ire wrote: > On 2017/09/01 10:30:59, juliandoucette wrote: > > NIT: I would prefer to order these in terms of specificity e.g. *, html, body, > > ... > > I think it makes more sense in terms of readability to have box-sizing: > border-box to come before box-sizing: inherit, particularly since the styles are > grouped by purpose, but I don't feel particularly strongly about it so changed. Acknowledged. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
