Skip to content
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

Fix service navigation mobile toggle spacing #5278

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

querkmachine
Copy link
Member

Fixes the top margin of the Service navigation component's mobile toggle button being missing when navigation is not alongside a service name.

This issue was introduced when making some eleventh-hour CSS changes to the component prior to release yesterday.

Changes

  • Changes the margin on the toggle button to apply to both the top and bottom of the button.
  • Adds a (rather lengthy 😬) sibling selector to check if a service name is being used and, if so, to remove the top margin of the toggle button.

Screenshots

(Screenshots have the GOV.UK header added to better illustrate the spacing issue.)

Before After
Without service name List of links with a button at the top, the button is pressed right at the edge of the header List of links with a button at the top, the button is has some spacing between it and the header
With service name List of links with a button at the top and a service name above that, all of them are spaced apart from one another and the header An image that appears identical to the previous 'before' image

@querkmachine querkmachine requested review from CharlotteDowns and a team August 30, 2024 12:58
@querkmachine querkmachine self-assigned this Aug 30, 2024
Copy link

github-actions bot commented Aug 30, 2024

📋 Stats

File sizes

File Size
dist/govuk-frontend-development.min.css 118.52 KiB
dist/govuk-frontend-development.min.js 43.63 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js 90.19 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs 84.69 KiB
packages/govuk-frontend/dist/govuk/all.mjs 1.05 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs 359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.css 118.5 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js 43.62 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs 5.55 KiB
packages/govuk-frontend/dist/govuk/init.mjs 4.97 KiB

Modules

File Size (bundled) Size (minified)
all.mjs 81.8 KiB 41.48 KiB
accordion.mjs 23.5 KiB 12.39 KiB
button.mjs 5.98 KiB 2.69 KiB
character-count.mjs 22.4 KiB 9.92 KiB
checkboxes.mjs 5.83 KiB 2.83 KiB
error-summary.mjs 7.89 KiB 3.46 KiB
exit-this-page.mjs 17.1 KiB 9.26 KiB
header.mjs 4.46 KiB 2.6 KiB
notification-banner.mjs 6.26 KiB 2.62 KiB
password-input.mjs 15.15 KiB 7.25 KiB
radios.mjs 4.83 KiB 2.38 KiB
service-navigation.mjs 4.46 KiB 2.69 KiB
skip-link.mjs 4.39 KiB 2.18 KiB
tabs.mjs 10.05 KiB 6.06 KiB

View stats and visualisations on the review app


Action run for acc9d12

Copy link

github-actions bot commented Aug 30, 2024

Stylesheets changes to npm package

diff --git a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
index bf4e7565a..1bbb7da0a 100644
--- a/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
+++ b/packages/govuk-frontend/dist/govuk/govuk-frontend.min.css
@@ -5309,7 +5309,7 @@ screen and (forced-colors:active) {
     font-size: 1rem;
     line-height: 1.25;
     display: inline-flex;
-    margin: 0 0 10px;
+    margin: 10px 0;
     padding: 0;
     border: 0;
     color: #1a65a6;
@@ -5379,6 +5379,10 @@ screen and (forced-colors:active) {
     display: none
 }
 
+.govuk-service-navigation__service-name+.govuk-service-navigation__wrapper .govuk-service-navigation__toggle {
+    margin-top: 0
+}
+
 .govuk-service-navigation__list {
     font-family: GDS Transport, arial, sans-serif;
     -webkit-font-smoothing: antialiased;

Action run for acc9d12

Copy link

github-actions bot commented Aug 30, 2024

Other changes to npm package

diff --git a/packages/govuk-frontend/dist/govuk/components/service-navigation/_index.scss b/packages/govuk-frontend/dist/govuk/components/service-navigation/_index.scss
index 0eb5a7ed3..ec8973231 100644
--- a/packages/govuk-frontend/dist/govuk/components/service-navigation/_index.scss
+++ b/packages/govuk-frontend/dist/govuk/components/service-navigation/_index.scss
@@ -90,7 +90,7 @@
   .govuk-service-navigation__toggle {
     @include govuk-font($size: 19, $weight: bold);
     display: inline-flex;
-    margin: 0 0 govuk-spacing(2);
+    margin: govuk-spacing(2) 0;
     padding: 0;
     border: 0;
     color: $govuk-service-navigation-link-colour;
@@ -117,6 +117,12 @@
     &[hidden] {
       display: none;
     }
+
+    // If we have both a service name and navigation toggle, remove the
+    // margin-top so that there isn't a bunch of space between them
+    .govuk-service-navigation__service-name + .govuk-service-navigation__wrapper & {
+      margin-top: 0;
+    }
   }
 
   .govuk-service-navigation__list {

Action run for acc9d12

Fixes the top margin of the service navigation component's mobile toggle button being too small when not used alongside a service name.
@querkmachine querkmachine force-pushed the fix-service-nav-toggle-spacing branch from 61f8e3e to acc9d12 Compare August 30, 2024 13:14
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-5278 August 30, 2024 13:15 Inactive
@querkmachine querkmachine marked this pull request as ready for review August 30, 2024 13:21
Copy link
Contributor

@owenatgov owenatgov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Sorry for the margin drama 🙈

Copy link
Contributor

@CharlotteDowns CharlotteDowns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants