Issue 29581874: Noissue - Removed website-default container widths (Closed)
|
Created:
Oct. 17, 2017, 3:13 p.m. by juliandoucette Modified:
Oct. 23, 2017, 1:33 p.m. Reviewers:
ire Base URL:
https://hg.adblockplus.org/website-defaults Visibility:
Public. |
DescriptionWe discussed this before and you wern't a fan of these. I'm not a fan of these either. Let's see how things work without them and then re-add them if necessary?
Patch Set 1 #
Total comments: 5
Patch Set 2 : Addressed comments #
Total comments: 10
Patch Set 3 : Fixed mistakes #
Total comments: 4
Patch Set 4 : Addressed comments #MessagesTotal messages: 15
(Context: I decided to try/propose this after being unimpressed by the excessive space around new abp.org #content.container.)
> We discussed this before and you wern't a fan of these. I'm not a fan of these either. Let's see how things work without them and then re-add them if necessary? :D On 2017/10/17 15:29:10, juliandoucette wrote: > (Context: I decided to try/propose this after being unimpressed by the excessive > space around new http://abp.org #content.container.) Ack. I had the same thought actually, was going to bring it up later
https://codereview.adblockplus.org/29581874/diff/29581875/static/scss/_base.scss File static/scss/_base.scss (right): https://codereview.adblockplus.org/29581874/diff/29581875/static/scss/_base.s... static/scss/_base.scss:44: width: $container-max-width; Since this is being applied to width, I think the variable name should be $container-width. Otherwise, it should be applied to the `max-width` property instead. https://codereview.adblockplus.org/29581874/diff/29581875/static/scss/_base.s... static/scss/_base.scss:47: padding: 0px $small-space; NIT: I think this space should be larger on bigger devices. Suggest: > $tablet-breakpoint = $medium-space
Thank you for the suggestions. I will take them. https://codereview.adblockplus.org/29581874/diff/29581875/static/scss/_base.scss File static/scss/_base.scss (right): https://codereview.adblockplus.org/29581874/diff/29581875/static/scss/_base.s... static/scss/_base.scss:44: width: $container-max-width; On 2017/10/17 17:34:37, ire wrote: > Since this is being applied to width, I think the variable name should be > $container-width. Otherwise, it should be applied to the `max-width` property > instead. Acknowledged. https://codereview.adblockplus.org/29581874/diff/29581875/static/scss/_base.s... static/scss/_base.scss:47: padding: 0px $small-space; On 2017/10/17 17:34:37, ire wrote: > NIT: I think this space should be larger on bigger devices. > > Suggest: > $tablet-breakpoint = $medium-space Acknowledged.
Ready for review. https://codereview.adblockplus.org/29581874/diff/29581875/static/scss/_base.scss File static/scss/_base.scss (right): https://codereview.adblockplus.org/29581874/diff/29581875/static/scss/_base.s... static/scss/_base.scss:47: padding: 0px $small-space; On 2017/10/17 17:34:37, ire wrote: > NIT: I think this space should be larger on bigger devices. > > Suggest: > $tablet-breakpoint = $medium-space I'm skeptical about $tablet-breakpoint because it's hard to fit 2 columns (or worse three) on tablets. I think that $large-desktop-width is the safest bet because wide screens (in particular) can afford wide margins (padding). How does this affect Help Center? e.g. Are there supposed to be (or should there be) wider margins anywhere? https://codereview.adblockplus.org/29581874/diff/29582634/static/scss/_base.scss File static/scss/_base.scss (right): https://codereview.adblockplus.org/29581874/diff/29582634/static/scss/_base.s... static/scss/_base.scss:42: @media(min-width: $large-desktop-breakpoint) I wasn't sure what to call this breakpoint. As a result, I decided to use existing widths and change them to some sort of $container variable later if necessary.
https://codereview.adblockplus.org/29581874/diff/29582634/static/scss/_base.scss File static/scss/_base.scss (right): https://codereview.adblockplus.org/29581874/diff/29582634/static/scss/_base.s... static/scss/_base.scss:34: width: $large-desktop-width; I think you should still use a $container-width variable. This way, it can be changed/overridden without having to modify the .container class https://codereview.adblockplus.org/29581874/diff/29582634/static/scss/_base.s... static/scss/_base.scss:42: @media(min-width: $large-desktop-breakpoint) On 2017/10/18 13:44:33, juliandoucette wrote: > I wasn't sure what to call this breakpoint. As a result, I decided to use > existing widths and change them to some sort of $container variable later if > necessary. I think the breakpoint name works https://codereview.adblockplus.org/29581874/diff/29582634/static/scss/_base.s... static/scss/_base.scss:42: @media(min-width: $large-desktop-breakpoint) I was wondering why this style wasn't applied, looks like you put the media query outside the .container selector rules :P https://codereview.adblockplus.org/29581874/diff/29582634/static/scss/_base.s... static/scss/_base.scss:42: @media(min-width: $large-desktop-breakpoint) I think applying this style at $large-desktop-breakpoint is too large. The padding is never actually relevant because at the point at which the device width is small enough for the .container to touch the edges, the device width is less than $large-desktop-breakpoint https://codereview.adblockplus.org/29581874/diff/29582634/static/scss/_variab... File static/scss/_variables.scss (left): https://codereview.adblockplus.org/29581874/diff/29582634/static/scss/_variab... static/scss/_variables.scss:89: // Grid breakpoints Why were these removed? The variables are still being used in _grid.scss so the CSS is no longer compiling
This is embarrassing... :D - #julianisonit https://codereview.adblockplus.org/29581874/diff/29582634/static/scss/_base.scss File static/scss/_base.scss (right): https://codereview.adblockplus.org/29581874/diff/29582634/static/scss/_base.s... static/scss/_base.scss:42: @media(min-width: $large-desktop-breakpoint) On 2017/10/20 07:49:38, ire wrote: > I was wondering why this style wasn't applied, looks like you put the media > query outside the .container selector rules :P *I do this all the time D:* https://codereview.adblockplus.org/29581874/diff/29582634/static/scss/_base.s... static/scss/_base.scss:42: @media(min-width: $large-desktop-breakpoint) On 2017/10/20 07:49:38, ire wrote: > I think applying this style at $large-desktop-breakpoint is too large. > > The padding is never actually relevant because at the point at which the device > width is small enough for the .container to touch the edges, the device width is > less than $large-desktop-breakpoint You're right. I seem to be confused here. I was thinking about the space in between columns being larger at large-desktop-width. But I wasn't styling that space :D https://codereview.adblockplus.org/29581874/diff/29582634/static/scss/_variab... File static/scss/_variables.scss (left): https://codereview.adblockplus.org/29581874/diff/29582634/static/scss/_variab... static/scss/_variables.scss:89: // Grid breakpoints On 2017/10/20 07:49:39, ire wrote: > Why were these removed? The variables are still being used in _grid.scss so the > CSS is no longer compiling I didn't remove these manually - which means I forgot to hg update first or something.
I simplified this PatchSet because I don't know if/how I want to apply a larger
.container padding at larger sizes. e.g.
- At what size(s) should there be a larger padding?
- Note: I could apply this padding more programmatically if there are more
than one e.g. foreach breakpoint apply padding
- Will this larger padding(s) apply to all of our website designs?
- Note: If not, it may be a better idea to leave this out of
website-defaults so that we don't have to override it
A couple minor things. On 2017/10/20 11:18:52, juliandoucette wrote: > I simplified this PatchSet because I don't know if/how I want to apply a larger > .container padding at larger sizes. e.g. > > - At what size(s) should there be a larger padding? > - Note: I could apply this padding more programmatically if there are more > than one e.g. foreach breakpoint apply padding > - Will this larger padding(s) apply to all of our website designs? > - Note: If not, it may be a better idea to leave this out of > website-defaults so that we don't have to override it I think this simplified version works best for website-defaults. https://codereview.adblockplus.org/29581874/diff/29582634/static/scss/_base.scss File static/scss/_base.scss (right): https://codereview.adblockplus.org/29581874/diff/29582634/static/scss/_base.s... static/scss/_base.scss:42: @media(min-width: $large-desktop-breakpoint) On 2017/10/20 11:05:11, juliandoucette wrote: > On 2017/10/20 07:49:38, ire wrote: > > I was wondering why this style wasn't applied, looks like you put the media > > query outside the .container selector rules :P > > *I do this all the time D:* Haha :P https://codereview.adblockplus.org/29581874/diff/29584564/static/scss/_base.scss File static/scss/_base.scss (right): https://codereview.adblockplus.org/29581874/diff/29584564/static/scss/_base.s... static/scss/_base.scss:37: margin-right: auto; margin-right should come before margin-left (same for padding) according to our coding style https://codereview.adblockplus.org/29581874/diff/29584564/static/scss/_variab... File static/scss/_variables.scss (right): https://codereview.adblockplus.org/29581874/diff/29584564/static/scss/_variab... static/scss/_variables.scss:107: $container-width: $large-desktop-width; Missing !default
Thanks! https://codereview.adblockplus.org/29581874/diff/29584564/static/scss/_base.scss File static/scss/_base.scss (right): https://codereview.adblockplus.org/29581874/diff/29584564/static/scss/_base.s... static/scss/_base.scss:37: margin-right: auto; On 2017/10/23 11:59:37, ire wrote: > margin-right should come before margin-left (same for padding) according to our > coding style Done. https://codereview.adblockplus.org/29581874/diff/29584564/static/scss/_variab... File static/scss/_variables.scss (right): https://codereview.adblockplus.org/29581874/diff/29584564/static/scss/_variab... static/scss/_variables.scss:107: $container-width: $large-desktop-width; On 2017/10/23 11:59:37, ire wrote: > Missing !default Done.
On 2017/10/23 12:45:49, juliandoucette wrote: > Thanks! > > https://codereview.adblockplus.org/29581874/diff/29584564/static/scss/_base.scss > File static/scss/_base.scss (right): > > https://codereview.adblockplus.org/29581874/diff/29584564/static/scss/_base.s... > static/scss/_base.scss:37: margin-right: auto; > On 2017/10/23 11:59:37, ire wrote: > > margin-right should come before margin-left (same for padding) according to > our > > coding style > > Done. > > https://codereview.adblockplus.org/29581874/diff/29584564/static/scss/_variab... > File static/scss/_variables.scss (right): > > https://codereview.adblockplus.org/29581874/diff/29584564/static/scss/_variab... > static/scss/_variables.scss:107: $container-width: $large-desktop-width; > On 2017/10/23 11:59:37, ire wrote: > > Missing !default > > Done. You didn't upload the new patchset?
> You didn't upload the new patchset? I got distracted. Done.
LGTM
|
