-
Notifications
You must be signed in to change notification settings - Fork 338
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Header menu button position refactor #4150
Conversation
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for 8d9d07d |
right: 0; | ||
max-width: $govuk-header-menu-button-width; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @owenatgov
My only hesitation would be around the new max-width: 80px
restriction.
Teams can customise params.menuButtonLabel
so full width with a logo-sized safe area would be better? Probably a lot easier to do once #1739 lands
![Menu button with long name](https://private-user-images.githubusercontent.com/415517/264969245-f18d7d47-c6f2-44f3-a76e-989047967e7a.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MzkxMTkzMTksIm5iZiI6MTczOTExOTAxOSwicGF0aCI6Ii80MTU1MTcvMjY0OTY5MjQ1LWYxOGQ3ZDQ3LWM2ZjItNDRmMy1hNzZlLTk4OTA0Nzk2N2U3YS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjA5JTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIwOVQxNjM2NTlaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT0xZGNiMDhhN2U1YmIyYzNmMjZlYTIxNTQ2ZDEzYjRkNzQxZDlkN2FlYjNkNGQ2ZWEyZmYwYzVhM2E1OTdlZjc5JlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.xKdr5ewOkTMYhL5gQFKxWQYH42b4LGg0z7buelqFnWs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a comment in e9d62b4 that the only instance of a service not using the text "menu" that's publicly available is the Welsh for menu: "Dewislen". However, that's a good point, we could just turn the calculation the other way around. I had a go at "shrink wrapping" the logo's width but was having trouble with it. I also need to have a think about how we could set the padding to the rest of the width container; I'm sure I've managed to do this before? Let me muck around with this. If you have any ideas I am very open to them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's what I was thinking
Including margins, the logo occupies 183px width (or 191px using Arial)
Will need to adjust for potential text-only zoom, but given a 200px logo "safe area" we could:
--- a/packages/govuk-frontend/src/govuk/components/header/_index.scss
+++ b/packages/govuk-frontend/src/govuk/components/header/_index.scss
@@ -10,6 +10,7 @@
// This crown height is only used to calculate top offset of mobile menu button
// as the crown svg height is the only thing that controls the height of the header
$govuk-header-crown-height: 30px;
+ $govuk-header-logo-width: 200px;
$govuk-header-menu-button-height: 20px;
$govuk-header-menu-button-width: 80px;
@@ -83,6 +84,7 @@
@include govuk-typography-common;
@include govuk-link-style-inverse;
+ max-width: $govuk-header-logo-width;
text-decoration: none;
&:hover {
@@ -182,13 +184,16 @@
top: (((govuk-spacing($govuk-header-vertical-spacing-value) * 2) + $govuk-header-crown-height) / 2) -
($govuk-header-menu-button-height / 2);
right: 0;
- max-width: $govuk-header-menu-button-width;
+ // Protect the absolute positioned menu button from overlapping with the
+ // logo with left position using the logo's width
+ left: $govuk-header-logo-width;
min-height: $govuk-header-menu-button-height;
margin: 0;
padding: 0;
border: 0;
color: govuk-colour("white");
background: none;
+ text-align: right;
cursor: pointer;
&:hover {
@@ -215,6 +220,7 @@
@include govuk-media-query($from: tablet) {
top: govuk-spacing(3);
+ left: auto;
}
.govuk-frontend-supported & {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Downsides are that the button's clickable area will grow to fill non-logo space
Not a bad thing at mobile size, but could fix the width at tablet size perhaps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed up a branch header-menu-button-position-refactor-colin with commit 4b02738
a78cd75
to
061e005
Compare
@owenatgov Mind rebasing this with Mainly to pick up these two: |
061e005
to
679538e
Compare
Stylesheets changes to npm packagediff --git a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
index 403e85fb3..101c661d1 100644
--- a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
+++ b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
@@ -3876,7 +3876,7 @@ only screen and (min-resolution:2dppx) {
.govuk-header__logo {
margin-bottom: 10px;
- padding-right: 50px
+ padding-right: 80px
}
@media (min-width:48.0625em) {
@@ -3908,13 +3908,16 @@ only screen and (min-resolution:2dppx) {
font-size: .875rem;
line-height: 1.1428571429;
position: absolute;
- top: 20px;
+ top: 13px;
right: 0;
+ max-width: 80px;
+ min-height: 24px;
margin: 0;
padding: 0;
border: 0;
color: #fff;
background: none;
+ word-break: break-all;
cursor: pointer
}
Action run for 8d9d07d |
Other changes to npm packagediff --git a/packages/govuk-frontend/dist/govuk/components/header/_index.scss b/packages/govuk-frontend/dist/govuk/components/header/_index.scss
index 919f708d4..de025ff07 100644
--- a/packages/govuk-frontend/dist/govuk/components/header/_index.scss
+++ b/packages/govuk-frontend/dist/govuk/components/header/_index.scss
@@ -6,6 +6,12 @@
$govuk-header-link-active: #1d8feb;
$govuk-header-nav-item-border-color: #2e3133;
$govuk-header-link-underline-thickness: 3px;
+ $govuk-header-vertical-spacing-value: 2;
+ // This crown height is only used to calculate top offset of mobile menu button
+ // as the crown svg height is the only thing that controls the height of the header
+ $govuk-header-crown-height: 30px;
+ $govuk-header-menu-button-height: 24px;
+ $govuk-header-menu-button-width: 80px;
.govuk-header {
@include govuk-font($size: 16, $line-height: 1);
@@ -28,7 +34,7 @@
@include govuk-clearfix;
position: relative;
margin-bottom: -$govuk-header-border-width;
- padding-top: govuk-spacing(2);
+ padding-top: govuk-spacing($govuk-header-vertical-spacing-value);
border-bottom: $govuk-header-border-width solid $govuk-header-border-color;
}
@@ -160,8 +166,10 @@
}
.govuk-header__logo {
- @include govuk-responsive-margin(2, "bottom");
- padding-right: govuk-spacing(8);
+ @include govuk-responsive-margin($govuk-header-vertical-spacing-value, "bottom");
+ // Protect the absolute positioned menu button from overlapping with the
+ // logo with right padding using the button's width
+ padding-right: $govuk-header-menu-button-width;
@include govuk-media-query($from: desktop) {
width: 33.33%;
@@ -189,13 +197,22 @@
.govuk-header__menu-button {
@include govuk-font($size: 16);
position: absolute;
- top: govuk-spacing(4);
+ // calculate top offset by:
+ // - getting the vertical spacing for the top and the bottom of the header
+ // - adding that to the crown height
+ // - dividing it by 2 so you have the vertical centre of the header
+ // - subtracting half the height of the menu button
+ top: (((govuk-spacing($govuk-header-vertical-spacing-value) * 2) + $govuk-header-crown-height) / 2) -
+ ($govuk-header-menu-button-height / 2);
right: 0;
+ max-width: $govuk-header-menu-button-width;
+ min-height: $govuk-header-menu-button-height;
margin: 0;
padding: 0;
border: 0;
color: govuk-colour("white");
background: none;
+ word-break: break-all;
cursor: pointer;
&:hover {
Action run for 8d9d07d |
679538e
to
a39c863
Compare
We do this by assigning the menu button a fixed height, getting half the calculated height of the header (the vertical spacing plus the crown height) minus half the button height. This ensures that we'll always get the correct top offset even if we move these values around. The crown height variable is technically unnecessary but the crown svg is the only thing controlling the base height of the header and putting it in a variable makes it easier to find.
This value is applied to a component level variable and used to additionally calculate the right padding of the govuk logo container. 80px was chosen as a github search at time of commit suggests that the only time "Menu" is replaced is when the page is in Welsh, in which case it's replaced with "Dewislen". Dewislen at 16px (anticipating the new typography scale) is roughly 80px.
Prevents individual very long words from overflowing off the page and breaking the match media
Allows us to resolve #4357 at the same time
a39c863
to
8d9d07d
Compare
// as the crown svg height is the only thing that controls the height of the header | ||
$govuk-header-crown-height: 30px; | ||
$govuk-header-menu-button-height: 24px; | ||
$govuk-header-menu-button-width: 80px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think I'm happy to approve, is this width definitely enough?
E.g. "Menu" in Welsh, quick double check?
@colinrotherham Good shout. Testing with "Dewislen" (menu in Welsh) and increase the font size to 16px which ti will be with the new typography scale, get a total with of 78.359px width on the button. I'm feeling positive so I'm gonna be cheeky and merge. |
…factor Header menu button position refactor
What/Why
Refactor the header component's navigation menu button styling:
Additional details can be found in comments within commits.
Resolves #3896
Reimplementation off of #4065
Thoughts
I couldn't figure out a good alternative to using right hand padding on the logo and an absolute positioned menu button. Ideally we would dynamically calculate the padding but sass doesn't have the means to fetch dynamic width. This way at least there's a relationship between the padding and the button width and it's easier to change the padding/button width. Open to ideas.
I'm wondering if there's a more explicit way to define the height of the header or if our assumption that the crown is always 30px is enough. We want it to stay at 50px but that's entirely human memory driven. There's nothing in our code to reflect that. Nothing to lose sleep over.