Issue 29555879: Noissue - Center the content and unfix on small screens the sidebar (Closed)
|
Created:
Sept. 25, 2017, 9:58 p.m. by saroyanm Modified:
Sept. 27, 2017, 5:09 p.m. Reviewers:
Thomas Greiner CC:
ire Visibility:
Public. |
DescriptionNoissue - Center the content and unfix on small screens the sidebar
Patch Set 1 #
Total comments: 7
Patch Set 2 : Rebased #Patch Set 3 : Added comment #MessagesTotal messages: 6
https://codereview.adblockplus.org/29555879/diff/29555880/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29555879/diff/29555880/skin/new-options.cs... skin/new-options.css:523: @media (min-height: 37rem) @Thomas can you please send the link to the solution you mentioned to me today, so I'll adapt this accordingly.
https://codereview.adblockplus.org/29555879/diff/29555880/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29555879/diff/29555880/skin/new-options.cs... skin/new-options.css:523: @media (min-height: 37rem) On 2017/09/25 22:02:13, saroyanm wrote: > @Thomas can you please send the link to the solution you mentioned to me today, > so I'll adapt this accordingly. What I was mentioning was `position: sticky` but according to MDN it doesn't look like we can use it yet because it has only been introduced in Chrome in version 56. https://developer.mozilla.org/en-US/docs/Web/CSS/position#Browser_compatibility https://codereview.adblockplus.org/29555879/diff/29555880/skin/new-options.cs... skin/new-options.css:523: @media (min-height: 37rem) This is a bit of a hack because it depends on the height of the sidebar contents which may change. Why do we care about this in the first place? We don't need to optimize for smaller screens yet. If we did we should probably work on other stuff to fix this, such as introducing a scrollable hamburger menu and using proper font sizes that make better use of the available screen real estate.
https://codereview.adblockplus.org/29555879/diff/29555880/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29555879/diff/29555880/skin/new-options.cs... skin/new-options.css:523: @media (min-height: 37rem) On 2017/09/26 10:40:38, Thomas Greiner wrote: > This is a bit of a hack because it depends on the height of the sidebar contents > which may change. > > Why do we care about this in the first place? We don't need to optimize for > smaller screens yet. If we did we should probably work on other stuff to fix > this, such as introducing a scrollable hamburger menu and using proper font > sizes that make better use of the available screen real estate. As we did introduce fixed root element and prevented people from increasing the font-size, this solution will fix the edge case of zooming using `ctrl + "+"`. Also considering translations being long this can be a good help for us to manage the "stickiness:)". Alternative 1: I could use two Absolute layouts and Z-index and hide the footer behind tabs, which will look funny. Alternative2: I can use JavaScript to make the footer unstick. I can't see much more options right now, but my favoite would be not to introduce footer in sidebar that is pushed to the bottom. https://codereview.adblockplus.org/29555879/diff/29555880/skin/new-options.cs... skin/new-options.css:523: @media (min-height: 37rem) On 2017/09/26 10:40:38, Thomas Greiner wrote: > On 2017/09/25 22:02:13, saroyanm wrote: > > @Thomas can you please send the link to the solution you mentioned to me > today, > > so I'll adapt this accordingly. > What I was mentioning was `position: sticky` but according to MDN it doesn't > look like we can use it yet because it has only been introduced in Chrome in > version 56. > https://developer.mozilla.org/en-US/docs/Web/CSS/position#Browser_compatibility I see.
https://codereview.adblockplus.org/29555879/diff/29555880/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29555879/diff/29555880/skin/new-options.cs... skin/new-options.css:523: @media (min-height: 37rem) On 2017/09/26 16:55:48, saroyanm wrote: > As we did introduce fixed root element and prevented people from increasing the > font-size, this solution will fix the edge case of zooming using `ctrl + "+"`. > Also considering translations being long this can be a good help for us to > manage the "stickiness:)". > Alternative 1: I could use two Absolute layouts and Z-index and hide the footer > behind tabs, which will look funny. > Alternative2: I can use JavaScript to make the footer unstick. > > I can't see much more options right now, but my favoite would be not to > introduce footer in sidebar that is pushed to the bottom. Ok, since there's no good solution that we can implement quickly, let's leave it as is. However, please add a comment to the code to explain why this workaround is necessary.
https://codereview.adblockplus.org/29555879/diff/29555880/skin/new-options.css File skin/new-options.css (right): https://codereview.adblockplus.org/29555879/diff/29555880/skin/new-options.cs... skin/new-options.css:523: @media (min-height: 37rem) On 2017/09/27 13:27:50, Thomas Greiner wrote: > On 2017/09/26 16:55:48, saroyanm wrote: > > As we did introduce fixed root element and prevented people from increasing > the > > font-size, this solution will fix the edge case of zooming using `ctrl + "+"`. > > Also considering translations being long this can be a good help for us to > > manage the "stickiness:)". > > Alternative 1: I could use two Absolute layouts and Z-index and hide the > footer > > behind tabs, which will look funny. > > Alternative2: I can use JavaScript to make the footer unstick. > > > > I can't see much more options right now, but my favoite would be not to > > introduce footer in sidebar that is pushed to the bottom. > > Ok, since there's no good solution that we can implement quickly, let's leave it > as is. However, please add a comment to the code to explain why this workaround > is necessary. Agree, done.
LGTM 
|
