-
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
Service navigation component #5206
Conversation
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
483a18a
to
fcdb07e
Compare
fcdb07e
to
7026715
Compare
30f500f
to
3cdb499
Compare
3cdb499
to
a62ba40
Compare
2f46002
to
448355a
Compare
packages/govuk-frontend/src/govuk/components/service-navigation/README.md
Show resolved
Hide resolved
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 - have suggested some minor changes for style and clarity
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.
A few minor comments. We're looking good. I've got a couple of extra points to make outside code. One thing I think we should do before the launch party and 3 things I'm willing to negotiate on and park for later:
Gotta do
The commit history could do with a tidy. I suspect this was on your radar already but just to enshrine it as a thing we should do.
Don't gotta do now
Firstly, I think an example of the service nav would be good. Either replacing the header nav in an existing example or creating a new one showing the header and the service nav together. The latter would be my preference.
Secondly, there's currently no way to translate the service nav. This can be an iteration but it's a common pattern in our components so we should do it eventually.
Finally, I noticed on mobile that the margin collapse isn't working between the service name and the menu button, creating a very slightly jarring, bigger than expected space:
I know why this is happening: the margin for the button is applied inside the nav element and it doesn't bubble out to its parent. This can be solved with a bit of margin wrangling on mobile which is annoying but doable. I don't know if it's intentional and if it isn't it's not a reason not to merge, it just looks a bit weird to my eye.
packages/govuk-frontend/src/govuk/components/service-navigation/_index.scss
Outdated
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/service-navigation/_index.scss
Outdated
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/service-navigation/_index.scss
Outdated
Show resolved
Hide resolved
packages/govuk-frontend/src/govuk/components/service-navigation/service-navigation.yaml
Outdated
Show resolved
Hide resolved
@owenatgov Which parts cannot be translated? I can't see anything hardcoded at a glance. |
cc486de
to
276cff9
Compare
@querkmachine Ah whoops you're right! Sorry, habitual reviewing. |
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.
Code is looking sharp. Congratulations and well done ✨
I'm going to pop this note here that I unfortunately noticed another example of non-collapsing margin 🥲 This time at tablet when the nav itself overflows:

This one looks less weird and I suspect is even more annoying to fix so I don't think it's worth busting our backs to get rid of it given the time frames. Maybe worth reviewing post release when we've got a bit more time.
packages/govuk-frontend/src/govuk/components/service-navigation/README.md
Outdated
Show resolved
Hide resolved
@owenatgov Yeah, that one is a lot trickier as the wrapping is a result of |
276cff9
to
ba9dcfe
Compare
packages/govuk-frontend/src/govuk/components/service-navigation/README.md
Outdated
Show resolved
Hide resolved
Co-authored-by: seaemsi <[email protected]>
ba9dcfe
to
414298b
Compare
This PR supersedes #4950. This PR should be considered the working branch for the navigation work.
I opted to make a new PR as we've decided to remove or refactor many aspects of the previous PR. However, we may still integrate those aspects at a later date, so having the previous PR exist as-is as a point of reference and comparison is still useful.
Changes from the previous PR
Refactor navigation links so active styles applied directly to link/span, rather than the list itemnavigation__navigation
repetition<section>
element if a service name isn't present