Issue 29591555: Issue 5940 - Created default section styles (Closed)
|
Created:
Oct. 29, 2017, 1:13 p.m. by juliandoucette Modified:
Nov. 3, 2017, 11:08 a.m. Reviewers:
ire Base URL:
https://hg.adblockplus.org/website-defaults Visibility:
Public. |
DescriptionIssue 5940 - Created default section styles
Patch Set 1 #
Total comments: 6
MessagesTotal messages: 7
Thanks Julian! A few comments. https://codereview.adblockplus.org/29591555/diff/29591556/static/scss/_utilit... File static/scss/_utilities.scss (right): https://codereview.adblockplus.org/29591555/diff/29591556/static/scss/_utilit... static/scss/_utilities.scss:49: .section TOL: I'm a bit hesitant about this because this isn't exactly what I always want of a .section. For example, the .section in help.eyeo.com doesn't work this way, there's no padding, and there is only margin applied in one direction. If I think about it a lot, this definition of a .section is probably more accurate than my own implementation in hc, so maybe the solution is for me to change the classname I have to something else entirely. https://codereview.adblockplus.org/29591555/diff/29591556/static/scss/_utilit... static/scss/_utilities.scss:52: margin-top: $small-space; Using these variables has the same problem as using a non-unique variable for the .container width. It's not really that configurable because to change it is to change a global variable being used for other things. Maybe this needs its own variable? e.g. $section-mobile-spacing etc ? https://codereview.adblockplus.org/29591555/diff/29591556/static/scss/_utilit... static/scss/_utilities.scss:57: @media(min-width: $tablet-breakpoint) NIT: I think we should decide on if we put a space between the "@media" and the opening bracket. I think we should have a space, i.e. "@media (min-width: $tablet-breakpoint)"
Thanks Ire, I'll update this soon. Dono what I was thinking the first time :/. https://codereview.adblockplus.org/29591555/diff/29591556/static/scss/_utilit... File static/scss/_utilities.scss (right): https://codereview.adblockplus.org/29591555/diff/29591556/static/scss/_utilit... static/scss/_utilities.scss:49: .section On 2017/10/30 08:07:20, ire wrote: > TOL: I'm a bit hesitant about this because this isn't exactly what I always want > of a .section. For example, the .section in http://help.eyeo.com doesn't work this way, > there's no padding, and there is only margin applied in one direction. If I > think about it a lot, this definition of a .section is probably more accurate > than my own implementation in hc, so maybe the solution is for me to change the > classname I have to something else entirely. Nah, I think I like your section more. I want to be able to place these one after another or one after the navbar without space (margin) between. And the inner padding could be a problem for content with margin. https://codereview.adblockplus.org/29591555/diff/29591556/static/scss/_utilit... static/scss/_utilities.scss:52: margin-top: $small-space; On 2017/10/30 08:07:20, ire wrote: > Using these variables has the same problem as using a non-unique variable for > the .container width. It's not really that configurable because to change it is > to change a global variable being used for other things. Maybe this needs its > own variable? e.g. $section-mobile-spacing etc ? Agreed. https://codereview.adblockplus.org/29591555/diff/29591556/static/scss/_utilit... static/scss/_utilities.scss:57: @media(min-width: $tablet-breakpoint) On 2017/10/30 08:07:20, ire wrote: > NIT: I think we should decide on if we put a space between the "@media" and the > opening bracket. I think we should have a space, i.e. "@media (min-width: > $tablet-breakpoint)" Agreed.
On 2017/10/30 12:16:47, juliandoucette wrote: > Nah, I think I like your section more. I want to be able to place these one > after another or one after the navbar without space (margin) between. And the > inner padding could be a problem for content with margin. Afterthought: Actually, it seems I'm confusing 1. A layout section (no vertical space) 2. A content section (vertical space). I'll think on this a bit more and update one and/or the other if I think it's appropriate.
On 2017/10/30 12:19:02, juliandoucette wrote: > On 2017/10/30 12:16:47, juliandoucette wrote: > > Nah, I think I like your section more. I want to be able to place these one > > after another or one after the navbar without space (margin) between. And the > > inner padding could be a problem for content with margin. > > Afterthought: Actually, it seems I'm confusing 1. A layout section (no vertical > space) 2. A content section (vertical space). I'll think on this a bit more and > update one and/or the other if I think it's appropriate. Yes I think I am confusing/conflating the two as well. My interpretation of .section was a layout section, which pretty much only have a bottom-margin. I don't think we need to call this a .section, the classname could be more literal, something like .bottom-spacing . Whereas a .section could be the content section, which will have padding etc.
On 2017/10/31 08:06:53, ire wrote: > Yes I think I am confusing/conflating the two as well. > > My interpretation of .section was a layout section, which pretty much only have > a bottom-margin. I don't think we need to call this a .section, the classname > could be more literal, something like .bottom-spacing . Whereas a .section could > be the content section, which will have padding etc. I agree. I think that this may be a question of whether or not we want to use spacing classes or enforce more consistent spacing across components. e.g. There seems to be no one-size-fits-all spacing that applies to the new adblockplus.org section spacing. I think that we should table this for now.
On 2017/11/02 13:54:58, juliandoucette wrote: > On 2017/10/31 08:06:53, ire wrote: > > Yes I think I am confusing/conflating the two as well. > > > > My interpretation of .section was a layout section, which pretty much only > have > > a bottom-margin. I don't think we need to call this a .section, the classname > > could be more literal, something like .bottom-spacing . Whereas a .section > could > > be the content section, which will have padding etc. > > I agree. I think that this may be a question of whether or not we want to use > spacing classes or enforce more consistent spacing across components. e.g. > There seems to be no one-size-fits-all spacing that applies to the new > http://adblockplus.org section spacing. > > I think that we should table this for now. Ack. 
|
||||||||||||||||||||||||||||||||||||||||||||||
