static/scss/layout/_navbar.scss - Issue 29485575: Issue 5385 - Create Site Header Component for Help Center

Side by Side Diff: static/scss/layout/_navbar.scss

Issue 29485575: Issue 5385 - Create Site Header Component for Help Center (Closed)

Patch Set: Fix svgs, Implement search, Show searchbar for no-js Created July 17, 2017, 9:24 a.m.

Left:
Right:

Use n/p to move between diff chunks; N/P to move between comments.

Jump to:

View unified diff | Download patch

OLD	NEW
(Empty)
	1 // This file is part of help.eyeo.com.

	2 // Copyright (C) 2017 Eyeo GmbH

	3 //

	4 // help.eyeo.com is free software: you can redistribute it and/or modify

	5 // it under the terms of the GNU General Public License as published by

	6 // the Free Software Foundation, either version 3 of the License, or

	7 // (at your option) any later version.

	8 //

	9 // help.eyeo.com is distributed in the hope that it will be useful,

	10 // but WITHOUT ANY WARRANTY; without even the implied warranty of

	11 // MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the

	12 // GNU General Public License for more details.

	13 //

	14 // You should have received a copy of the GNU General Public License

	15 // along with help.eyeo.com. If not, see <http://www.gnu.org/licenses/>.

	16

	17 .navbar

	18 {

	19 padding-top: 12px;
	juliandoucette 2017/07/19 18:03:39 I suggest using em here so that this padding scale I suggest using em here so that this padding scales with font-size. ire 2017/07/21 10:23:31 This will interfere with the fixed height. However Show quoted text On 2017/07/19 18:03:39, juliandoucette wrote: > I suggest using em here so that this padding scales with font-size. This will interfere with the fixed height. However, if we aren't making the header position:fixed then it doesn't matter. We will discuss in a meeting. juliandoucette 2017/07/24 21:08:10 Acknowledged. Good point. Show quoted text On 2017/07/21 10:23:31, ire wrote: > On 2017/07/19 18:03:39, juliandoucette wrote: > > I suggest using em here so that this padding scales with font-size. > > This will interfere with the fixed height. However, if we aren't making the > header position:fixed then it doesn't matter. We will discuss in a meeting. Acknowledged. Good point.
	20 padding-bottom: 12px;

	21 color: $white;
	juliandoucette 2017/07/19 18:03:39 I think it makes sense to implement generic things I think it makes sense to implement generic things like `.navbar` in website-defaults. With that in mind, I suggest that color is too specific for this class. I would suggest applying it to #site-header or something like .navbar-inverse, .navbar-black, etc. The same goes for color properties within.
	22 background-color: $black;

	23 }

	24

	25 .navbar a

	26 {

	27 color: $white;

	28 }

	29

	30 .navbar-wrapper
	juliandoucette 2017/07/19 18:03:38 Why .navbar-wrapper and not a more generic .contai Why .navbar-wrapper and not a more generic .container? (Assuming you are familiar with this pattern.) ire 2017/07/21 10:23:31 Because this container is specific to the navbar o Show quoted text On 2017/07/19 18:03:38, juliandoucette wrote: > Why .navbar-wrapper and not a more generic .container? (Assuming you are > familiar with this pattern.) Because this container is specific to the navbar only. If I were to use container, the selector would be `.navbar .container` instead of just `.navbar-container`/`.navbar-wrapper`. juliandoucette 2017/07/24 21:08:10 What is specific about the navbar's container? Are Show quoted text On 2017/07/21 10:23:31, ire wrote: > Because this container is specific to the navbar only. If I were to use > container, the selector would be `.navbar .container` instead of just > `.navbar-container`/`.navbar-wrapper`. What is specific about the navbar's container? Aren't the navbar's contents contained in the same width as the primary content?
	31 {

	32 width: $navbar-width;
	juliandoucette 2017/07/19 18:03:39 Where is this width specified? --- (I think it's Where is this width specified? --- (I think it's too small) ire 2017/07/21 10:23:31 It isn't specified anywhere. I used the $dekstop-w Show quoted text On 2017/07/19 18:03:39, juliandoucette wrote: > Where is this width specified? > > --- > > (I think it's too small) It isn't specified anywhere. I used the $dekstop-width from website-defaults plus some extra spacing because the navbar extends further than the content. juliandoucette 2017/07/24 21:08:10 They look the same to me? Show quoted text On 2017/07/21 10:23:31, ire wrote: > It isn't specified anywhere. I used the $dekstop-width from website-defaults > plus some extra spacing because the navbar extends further than the content. They look the same to me? ire 2017/08/10 16:54:43 In this image (https://bytebucket.org/adblockplus/ Show quoted text On 2017/07/24 21:08:10, juliandoucette wrote: > On 2017/07/21 10:23:31, ire wrote: > > It isn't specified anywhere. I used the $dekstop-width from website-defaults > > plus some extra spacing because the navbar extends further than the content. > > They look the same to me? In this image (https://bytebucket.org/adblockplus/spec/raw/6eb0fc6e906a60ac76d93cf1ba9ae770e...), the width of the navbar seems to be wider than the width of the content. ire 2017/08/16 14:41:26 I spoke with Jeen and she confirmed that the navba Show quoted text On 2017/08/10 16:54:43, ire wrote: > On 2017/07/24 21:08:10, juliandoucette wrote: > > On 2017/07/21 10:23:31, ire wrote: > > > It isn't specified anywhere. I used the $dekstop-width from website-defaults > > > plus some extra spacing because the navbar extends further than the content. > > > > > They look the same to me? > > In this image > (https://bytebucket.org/adblockplus/spec/raw/6eb0fc6e906a60ac76d93cf1ba9ae770e...), > the width of the navbar seems to be wider than the width of the content. I spoke with Jeen and she confirmed that the navbar width should be wider than the content width
	33 max-width: $content-max-width;
	juliandoucette 2017/07/19 18:03:38 Where is this max-width specified? --- (And why Where is this max-width specified? --- (And why use effectively 10% margin left/right < $navbar-width instead of e.g. 1em) ire 2017/07/21 10:23:30 It isn't specified anywhere. (Are these values sup Show quoted text On 2017/07/19 18:03:38, juliandoucette wrote: > Where is this max-width specified? > > --- > > (And why use effectively 10% margin left/right < $navbar-width instead of e.g. > 1em) It isn't specified anywhere. (Are these values supposed to be specified in the spec or somewhere?) Using percentages in this way is just my personal preference. It feels more responsive to me because the size adapts as the screen size changes. Though if the way you mentioned is the preferred method, I'm happy to change it. juliandoucette 2017/07/24 21:08:10 On the one hand: yes On the other hand: that which Show quoted text On 2017/07/21 10:23:30, ire wrote: > It isn't specified anywhere. (Are these values supposed to be specified in the > spec or somewhere?) On the one hand: yes On the other hand: that which is not specified is open to interpretation What matters to me: that we support the same widths across websites Show quoted text > Using percentages in this way is just my personal preference. It feels more > responsive to me because the size adapts as the screen size changes. Though if > the way you mentioned is the preferred method, I'm happy to change it. This relates to one of my comments in another file. I prefer actual margin/padding personally. But I'd rather base the decision on something other than our personal preferences if possible. ire 2017/08/10 16:54:43 Agreed. I haven't been able to find any objective Show quoted text On 2017/07/24 21:08:10, juliandoucette wrote: > On 2017/07/21 10:23:30, ire wrote: > > Using percentages in this way is just my personal preference. It feels more > > responsive to me because the size adapts as the screen size changes. Though if > > the way you mentioned is the preferred method, I'm happy to change it. > > This relates to one of my comments in another file. I prefer actual > margin/padding personally. But I'd rather base the decision on something other > than our personal preferences if possible. Agreed. I haven't been able to find any objective evidence that one way is better than the other. Have you got any particular justifications? If there are no objective reasons for either way, then I'm happy to go with your way for consistency with other sites. ire 2017/08/16 14:41:26 Done. Show quoted text On 2017/08/10 16:54:43, ire wrote: > On 2017/07/24 21:08:10, juliandoucette wrote: > > On 2017/07/21 10:23:30, ire wrote: > > > Using percentages in this way is just my personal preference. It feels more > > > responsive to me because the size adapts as the screen size changes. Though > if > > > the way you mentioned is the preferred method, I'm happy to change it. > > > > This relates to one of my comments in another file. I prefer actual > > margin/padding personally. But I'd rather base the decision on something other > > than our personal preferences if possible. > > Agreed. I haven't been able to find any objective evidence that one way is > better than the other. Have you got any particular justifications? > > If there are no objective reasons for either way, then I'm happy to go with your > way for consistency with other sites. > Done.
	34 margin-right: auto;

	35 margin-left: auto;

	36 }

OLD	NEW

« static/scss/layout/_header.scss ('K') | « static/scss/layout/_header.scss ('k') | static/scss/main.scss » ('j') | templates/default.tmpl » ('J')