Issue 29538612: Issue 5607 - Namespace content styles in website-defaults (Closed)
|
Created:
Sept. 7, 2017, 12:04 p.m. by ire Modified:
Sept. 12, 2017, 2:50 p.m. Reviewers:
juliandoucette Base URL:
https://hg.adblockplus.org/website-defaults Visibility:
Public. |
DescriptionIssue 5607 - Namespace content styles in website-defaults
Patch Set 1 #
Total comments: 16
Patch Set 2 : Move global content styles to _reset.scss #
Total comments: 5
Patch Set 3 : Addressed NITs #
MessagesTotal messages: 8
Ready for review. https://codereview.adblockplus.org/29538612/diff/29538613/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29538612/diff/29538613/scss/_content.scss#... scss/_content.scss:27: .content I added this to all the styles here. There are a couple that may not only apply to .content though, see comments below https://codereview.adblockplus.org/29538612/diff/29538613/scss/_content.scss#... scss/_content.scss:98: strong Should this be global? https://codereview.adblockplus.org/29538612/diff/29538613/scss/_content.scss#... scss/_content.scss:116: a:visited This should probably be global https://codereview.adblockplus.org/29538612/diff/29538613/scss/_content.scss#... scss/_content.scss:133: img This should probably be global https://codereview.adblockplus.org/29538612/diff/29538613/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29538612/diff/29538613/templates/default.t... templates/default.tmpl:11: <div class="container content"> Since we want these styles applied to everything in website-defaults, I added the class here
Thanks Ire, almost there :) https://codereview.adblockplus.org/29538612/diff/29538613/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29538612/diff/29538613/scss/_content.scss#... scss/_content.scss:27: .content On 2017/09/07 12:08:23, ire wrote: > I added this to all the styles here. There are a couple that may not only apply > to .content though, see comments below Acknowledged. https://codereview.adblockplus.org/29538612/diff/29538613/scss/_content.scss#... scss/_content.scss:98: strong On 2017/09/07 12:08:23, ire wrote: > Should this be global? Yes, and... NIT: I think that we should consider the relative value "bolder" if it's supported alright across browsers because it's possible to have a <strong> in an already bold element. Of course, we can/should address this in a separate issue. https://codereview.adblockplus.org/29538612/diff/29538613/scss/_content.scss#... scss/_content.scss:105: font-size: $small-font; NIT: I think that we should consider a relative value (e.g. 80%) that is equal to $small-font. Of course, we can/should address this in a separate issue. https://codereview.adblockplus.org/29538612/diff/29538613/scss/_content.scss#... scss/_content.scss:116: a:visited Regarding what should and should not be global. Perhaps there is a simple rule here? e.g. all inline content elements should be global and block content elements should be namespaced. Can you think of any obvious exceptions? (I haven't yet.) https://codereview.adblockplus.org/29538612/diff/29538613/scss/_content.scss#... scss/_content.scss:133: img On 2017/09/07 12:08:23, ire wrote: > This should probably be global Acknowledged. https://codereview.adblockplus.org/29538612/diff/29538613/templates/default.tmpl File templates/default.tmpl (right): https://codereview.adblockplus.org/29538612/diff/29538613/templates/default.t... templates/default.tmpl:11: <div class="container content"> On 2017/09/07 12:08:23, ire wrote: > Since we want these styles applied to everything in website-defaults, I added > the class here Acknowledged.
Thanks :) https://codereview.adblockplus.org/29538612/diff/29538613/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29538612/diff/29538613/scss/_content.scss#... scss/_content.scss:98: strong On 2017/09/08 15:43:53, juliandoucette wrote: > On 2017/09/07 12:08:23, ire wrote: > > Should this be global? > > Yes, and... > > NIT: I think that we should consider the relative value "bolder" if it's > supported alright across browsers because it's possible to have a <strong> in an > already bold element. Of course, we can/should address this in a separate issue. Acknowledged. https://codereview.adblockplus.org/29538612/diff/29538613/scss/_content.scss#... scss/_content.scss:105: font-size: $small-font; On 2017/09/08 15:43:53, juliandoucette wrote: > NIT: I think that we should consider a relative value (e.g. 80%) that is equal > to $small-font. Of course, we can/should address this in a separate issue. Acknowledged. https://codereview.adblockplus.org/29538612/diff/29538613/scss/_content.scss#... scss/_content.scss:116: a:visited On 2017/09/08 15:43:53, juliandoucette wrote: > Regarding what should and should not be global. Perhaps there is a simple rule > here? e.g. all inline content elements should be global and block content > elements should be namespaced. Can you think of any obvious exceptions? (I > haven't yet.) This is a good rule. I can't think of any exceptions either.
https://codereview.adblockplus.org/29538612/diff/29538613/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29538612/diff/29538613/scss/_content.scss#... scss/_content.scss:98: strong On 2017/09/08 15:43:53, juliandoucette wrote: > On 2017/09/07 12:08:23, ire wrote: > > Should this be global? > > Yes, and... > > NIT: I think that we should consider the relative value "bolder" if it's > supported alright across browsers because it's possible to have a <strong> in an > already bold element. Of course, we can/should address this in a separate issue. See https://issues.adblockplus.org/ticket/5651
https://codereview.adblockplus.org/29538612/diff/29538613/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29538612/diff/29538613/scss/_content.scss#... scss/_content.scss:98: strong On 2017/09/11 11:29:16, ire wrote: > On 2017/09/08 15:43:53, juliandoucette wrote: > > On 2017/09/07 12:08:23, ire wrote: > > > Should this be global? > > > > Yes, and... > > > > NIT: I think that we should consider the relative value "bolder" if it's > > supported alright across browsers because it's possible to have a <strong> in > an > > already bold element. Of course, we can/should address this in a separate > issue. > > See https://issues.adblockplus.org/ticket/5651 Acknowledged. https://codereview.adblockplus.org/29538612/diff/29541693/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29538612/diff/29541693/scss/_content.scss#... scss/_content.scss:31: ******************************************************************************/ NIT: Are these lines too long now that they are indented? https://codereview.adblockplus.org/29538612/diff/29541693/scss/_reset.scss File scss/_reset.scss (right): https://codereview.adblockplus.org/29538612/diff/29541693/scss/_reset.scss#ne... scss/_reset.scss:175: a:hover, NIT: I'm not sure about this one. I think that we often style links without a hover/active/focus underline outside of content areas.
https://codereview.adblockplus.org/29538612/diff/29541693/scss/_content.scss File scss/_content.scss (right): https://codereview.adblockplus.org/29538612/diff/29541693/scss/_content.scss#... scss/_content.scss:31: ******************************************************************************/ On 2017/09/11 15:54:34, juliandoucette wrote: > NIT: Are these lines too long now that they are indented? They were longer by just 2 spaces, so I shortened them. https://codereview.adblockplus.org/29538612/diff/29541693/scss/_reset.scss File scss/_reset.scss (right): https://codereview.adblockplus.org/29538612/diff/29541693/scss/_reset.scss#ne... scss/_reset.scss:175: a:hover, On 2017/09/11 15:54:34, juliandoucette wrote: > NIT: I'm not sure about this one. I think that we often style links without a > hover/active/focus underline outside of content areas. I'm not sure either, but I put it here because I think all links should have some hover/active/focus state, regardless of where they are. Particularly if it's not clear that it's a link since there is no underline initially.
LGTM https://codereview.adblockplus.org/29538612/diff/29541693/scss/_reset.scss File scss/_reset.scss (right): https://codereview.adblockplus.org/29538612/diff/29541693/scss/_reset.scss#ne... scss/_reset.scss:175: a:hover, On 2017/09/12 07:47:06, ire wrote: > On 2017/09/11 15:54:34, juliandoucette wrote: > > NIT: I'm not sure about this one. I think that we often style links without a > > hover/active/focus underline outside of content areas. > > I'm not sure either, but I put it here because I think all links should have > some hover/active/focus state, regardless of where they are. Particularly if > it's not clear that it's a link since there is no underline initially. Acknowledged. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
