-
Notifications
You must be signed in to change notification settings - Fork 268
'Flat' implementation of the service navigation component #4668
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
Conversation
✅ You can preview this change here:
To edit notification comments on pull requests, go to your Netlify project configuration. |
e2f34ae to
a7a209e
Compare
|
That's pretty neat! These breadcrumbs work really nicely for navigating up the page hierarchy. 🙌🏻 Navigating down the page hierarchy requires to scroll down the page to the list of sub pages, though, which can be quite far, like on the Community page. We'll likely want to have a think on how to make it easier to access these pages if we want to drop the accordion. (Random though, could we have a second Service Navigation for the sub pages on mobile?) On the topic of building the structure for the breadcrumbs, thinking it could help if we'd associate a |
|
Associating a parent is a fairly common problem and https://github.com/tests-always-included/metalsmith-ancestry might helps sort us out if necessary |
|
@romaricpascal I did start by adding an I found it harder to make that work, however, as the template doesn't know which item within There might be (probably is?) some way of accessing the item or ancestors from within the template context, but this technique seemed to suffice for the purposes of a spike. |
a7a209e to
74d268d
Compare
Remove theme from page titles, except for those on patterns, so that they don't duplicate the last item in the breadcrumbs.
mia-allers-gds
left a comment
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.
All looks good to me! (Breadcrumb stuff is another PR, right? That still needs some working out)
| .app-width-container, | ||
| .govuk-service-navigation .govuk-width-container { |
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.
suggestion We may want to open an issue/PR to investigate if we could customise $govuk-page-width so that we don't have a custom selector. This feels like its own piece of work, though, so happy to ship this 😊
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'm reasonably confident that's an option since ac2cbdf as the examples no longer include the wider Design System CSS.
But agree we can do it later rather than as part of this PR.
The fact that we needed to apply padding to both `app-breadcrumbs` and `app-content` was a sign that the padding was not applied to the right elements and should instead be applied to their parent. This means that whatever gets added inside `app-side-pane__content`, the right amount of space will surround it.
Make the spacing on single page layouts consistent with the spacing on the side page pages. This includes moving the `app-width-container` outside of `<main>` so that the horizontal spacing is more easily applied to both the breadcrumbs and `<main>`
|
As the Service Navigation is the first component we use on the Design System site, we're now running into the issue that Safari 11.1 does not support the decimal
Users still get the information, so I'd be keen to not block the merging of this PR as the issue with decimal |
36degrees
left a comment
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.
LGTM, nice work cleaning everything up 👍🏻
|
|
||
| // Override the right padding normally used to reserve space for the | ||
| // mobile menu toggle. | ||
| padding-right: 0; |
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.
We can get rid of this once we update to 5.10.2, right?
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.
Yup, I think that's the padding being removed in 5.10.2. This PR will likely get merged after we update the site to 5.10.2 so we should remove this from the PR before we merge 😊
Moves the vertical padding in the white area where the content is displayed to a `app-main-wrapper` class. This ensures the spacing is consistent between `layout-pane` and `layout-single-page` so the breadcrumbs end up at the same level on all pages. The padding is applied as its own class in a similar fashion that we have `govuk-main-wrapper` in GOV.UK Frontend, but is made to match the spacing designed for the `layout-pane`. It also fixes the bottom padding having disappeared after the content on the pages using `layout-single-page`.
Ports the code of #4368 for the rendering of the subnavigation of each item of the Service Nav in a `<template>` element which will then be used by JavaScript. Co-Authored-By: [email protected]
Instantiate one component per section of the navigation to easily group the toggle button and subnavigation it reveals Co-Authored-By: [email protected]
As the mobile navigation is only available when JavaScript is loaded, and gets shown/hidden through the `hidden` HTML attribute, styles do not need to be gated by a media query. As the links live withing the Service Navigation, we can reuse its `govuk-service-navigation__link` styles for the interactive elements (subnavigation toggle and links) Co-Authored-By: [email protected]
Include the fix that makes `<template>` tags correctly handled by `element-permitted-content`. https://gitlab.com/html-validate/html-validate/-/issues/305
As of GOV.UK Frontend v5.10.2 [1] the logo in the header only has padding applied to the right if the menu button is present. As discussed in #4668 [2], this means that we can now remove this override from the Design System code. [1]: https://github.com/alphagov/govuk-frontend/releases/tag/v5.10.2 [2]: #4668 (comment)




Implements the service navigation component, but in a 'flat' manner that doesn't try to reproduce the bespoke accordion functionality.
Changes
Thoughts
Rather than change how navigation items are named internally, I've made it so that a new object with renamed keys is assembled directly prior to calling the service navigation Nunjucks macro. Reworking the internals is part of #4367, though doing this assembly might be necessary to set the active styles for links, regardless.