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

Make use of hidden in header navigation functionality #2727

Merged
merged 3 commits into from
Aug 4, 2022

Conversation

owenatgov
Copy link
Contributor

@owenatgov owenatgov commented Jul 25, 2022

Fixes #775

What

Replaces the header menu's default CSS based show/hide functionality with a reliance on the hidden attribute. Lifted from a similar change to the design system website navigation.

Additionally does not load the mobile menu UI if matchMedia is not present and instead presents the "no JS" view to those browsers (IE9 and below).

Why

To stop an unusable menu button appearing if CSS fails to load.

The choice to make matchMedia a required function comes from us needing a way to move hidden attributes around between screen sizes without having to rely on CSS overrides, which kicks the problem down the road for users without CSS. As our IE9 and below users are low, it's unlikely that they'll be on screens that would require the mobile menu and they can still use the menu regardless, this is a low risk change.

Upgrade notes

This change introduces a hidden attribute to the menu button on the header markup, meaning that teams that use the header but don't use our macros will need to update their markup. However the component still operates without the hidden attribute thanks to fallbacks in the CSS, this just means that teams won't get the benefit of hidden on no CSS and no JS views. In this respect we aren't introducing a regression so I recommend this go into the next release non-breaking release.

@owenatgov owenatgov added this to the 4.3.0 milestone Jul 25, 2022
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2727 July 25, 2022 16:37 Inactive
@owenatgov owenatgov changed the title Make use of hidden in header navigation functionality. Make use of hidden in header navigation functionality Jul 25, 2022
@owenatgov owenatgov force-pushed the hidden-attribute-on-header-menu-nav branch from d3ad78c to 7c534d0 Compare July 25, 2022 16:42
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2727 July 25, 2022 16:42 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2727 July 25, 2022 16:44 Inactive
CHANGELOG.md Outdated Show resolved Hide resolved
@owenatgov owenatgov marked this pull request as ready for review July 25, 2022 16:45
@owenatgov owenatgov requested a review from a team July 25, 2022 17:34
Copy link
Member

@querkmachine querkmachine left a comment

Choose a reason for hiding this comment

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

Left some suggestions. Other than the changelog entry probably needing rewriting this looks good and does what it says on the tin!

src/govuk/components/header/header.mjs Outdated Show resolved Hide resolved
src/govuk/components/header/header.mjs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/govuk/components/header/_index.scss Outdated Show resolved Hide resolved
src/govuk/components/header/header.mjs Show resolved Hide resolved
src/govuk/components/header/header.test.js Outdated Show resolved Hide resolved
src/govuk/components/header/header.test.js Outdated Show resolved Hide resolved
src/govuk/components/header/header.test.js Outdated Show resolved Hide resolved
src/govuk/components/header/header.test.js Outdated Show resolved Hide resolved
@36degrees 36degrees removed this from the v4.3.0 milestone Jul 26, 2022
@owenatgov owenatgov force-pushed the hidden-attribute-on-header-menu-nav branch from ad6918e to 9f74c78 Compare July 27, 2022 13:52
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2727 July 27, 2022 13:53 Inactive
@owenatgov owenatgov force-pushed the hidden-attribute-on-header-menu-nav branch from 9f74c78 to 41fbf07 Compare July 27, 2022 14:44
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2727 July 27, 2022 14:44 Inactive
@owenatgov owenatgov force-pushed the hidden-attribute-on-header-menu-nav branch from 41fbf07 to bdd65f0 Compare July 28, 2022 10:09
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2727 July 28, 2022 10:09 Inactive
@owenatgov owenatgov requested a review from 36degrees July 28, 2022 10:11
@owenatgov owenatgov force-pushed the hidden-attribute-on-header-menu-nav branch from bdd65f0 to 9e757bb Compare July 29, 2022 15:26
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2727 July 29, 2022 15:27 Inactive
@owenatgov owenatgov requested a review from 36degrees July 29, 2022 15:44
@owenatgov owenatgov force-pushed the hidden-attribute-on-header-menu-nav branch from 9e757bb to 5f9eda5 Compare August 2, 2022 10:03
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2727 August 2, 2022 10:03 Inactive
Sets the `hidden` attribute on the menu button by default so that the menu button doesn't appear on no js. Additionally sets `&hidden` to be `display: none` in CSS to get around old versions of IE which don't support `hidden`.
@owenatgov
Copy link
Contributor Author

I've made an issue to handle the inconsistent hidden CSS fallback discussed in the above discussions #2746

@owenatgov owenatgov force-pushed the hidden-attribute-on-header-menu-nav branch from 5f9eda5 to 7da4c63 Compare August 3, 2022 13:20
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2727 August 3, 2022 13:20 Inactive
@owenatgov owenatgov force-pushed the hidden-attribute-on-header-menu-nav branch from 7da4c63 to af64c4b Compare August 4, 2022 09:31
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2727 August 4, 2022 09:32 Inactive
We use the native `hidden` attribute here instead of classes as it reliably hides elements for screen readers and when CSS doesn't load, saving us the need to rely on CSS to manage the menu.

This change also introduces a variable to save the menu opened/closed state and a `matchMedia` for the desktop view as a way to save mobile menu opened/closed state if the user moves from small screen to large then back to small. If `matchMedia` isn't available then we don't bind any events and the user will get the "no JS" view of the header menu. As there are very few users using IE9 or below and it's very unlikely that any of them will be on a screen small enough to require the mobile menu, and they can still use the navigation, this is an acceptable change.
@owenatgov owenatgov force-pushed the hidden-attribute-on-header-menu-nav branch from af64c4b to c59f0d2 Compare August 4, 2022 10:33
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2727 August 4, 2022 10:33 Inactive
@owenatgov owenatgov merged commit d97a486 into main Aug 4, 2022
@owenatgov owenatgov deleted the hidden-attribute-on-header-menu-nav branch August 4, 2022 11:04
@querkmachine querkmachine mentioned this pull request Aug 9, 2022
peteryates added a commit to x-govuk/govuk-components that referenced this pull request Aug 11, 2022
This follows on from the corresponding upstream change[0][1], released
in 4.3.0

Co-authored-by: Frankie Roberto <frankie@frankieroberto.com>

[0] alphagov/govuk-frontend#2727
[1] https://github.com/alphagov/govuk-frontend/pull/2727/files#diff-dcc9d461905ec3f2d5c9a73c4c7bc09499b0c8265fe7e32c097b0e6649e1106fL64
peteryates added a commit to x-govuk/govuk-components that referenced this pull request Aug 11, 2022
This follows on from the corresponding upstream change[0][1], released
in 4.3.0

[0] alphagov/govuk-frontend#2727
[1] https://github.com/alphagov/govuk-frontend/pull/2727/files#diff-dcc9d461905ec3f2d5c9a73c4c7bc09499b0c8265fe7e32c097b0e6649e1106fL64

Co-authored-by: Frankie Roberto <frankie@frankieroberto.com>
peteryates added a commit to x-govuk/govuk-components that referenced this pull request Aug 21, 2022
This follows on from the corresponding upstream change[0][1], released
in 4.3.0

[0] alphagov/govuk-frontend#2727
[1] https://github.com/alphagov/govuk-frontend/pull/2727/files#diff-dcc9d461905ec3f2d5c9a73c4c7bc09499b0c8265fe7e32c097b0e6649e1106fL64

Co-authored-by: Frankie Roberto <frankie@frankieroberto.com>
peteryates added a commit to x-govuk/govuk-components that referenced this pull request Aug 22, 2022
This follows on from the corresponding upstream change[0][1], released
in 4.3.0

[0] alphagov/govuk-frontend#2727
[1] https://github.com/alphagov/govuk-frontend/pull/2727/files#diff-dcc9d461905ec3f2d5c9a73c4c7bc09499b0c8265fe7e32c097b0e6649e1106fL64

Co-authored-by: Frankie Roberto <frankie@frankieroberto.com>
36degrees added a commit that referenced this pull request Dec 16, 2022
The header imports the `Element.prototype.classList` polyfill but hasn’t actually called `classList` anywhere since we updated it to use the `hidden` attribute to toggle the menu visibility in 7217a7f (#2727).
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.

Header component has visible unusable menu button when CSS fail to load.
4 participants