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

Restores test and refactors component and test for layout-super-navigation-header #3939

Merged

Conversation

davidtrussler
Copy link
Contributor

@davidtrussler davidtrussler commented Mar 20, 2024

What

These changes restores a test that was temporarily removed and refactors both that and the component it relates to.

(Trello ticket: Refactor and improve recent change to layout-super-navigation-header component)

Why

When we made the changes in PR#3918 (Remove "Popular" links from super navigation header) we lost a recently added accessibility improvement, specifically to close the search menu when a keyboard user tabs out of that element. This was because this addition relied on the presence of links that were removed in the previous PR. We decided at the time to go ahead with that change and return to this to restore the lost behaviour.

This restores that by refactoring the original method dealing with that so that it doesn't rely on the presence of links but broadens the selector to include other tab-able elements. It also makes a small change to the original code for the search menu to make both more consistent with each other.

Copy link
Contributor

@MartinJJones MartinJJones left a comment

Choose a reason for hiding this comment

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

Nice work on adding this functionality back in to the super navigation menu.

The code itself looks good to me, just made some small suggestions around the naming of things.

@davidtrussler davidtrussler force-pushed the 2485_Refactor-layout-super-navigation-header-component branch from 0baa569 to a799630 Compare March 25, 2024 11:31
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3939 March 25, 2024 11:31 Inactive
@davidtrussler davidtrussler changed the title Restores test and refactors component and test Restores test and refactors component and test for layout-super-navigation-header Mar 25, 2024
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3939 March 25, 2024 11:35 Inactive
@davidtrussler
Copy link
Contributor Author

Nice work on adding this functionality back in to the super navigation menu.

The code itself looks good to me, just made some small suggestions around the naming of things.

Thanks @MartinJJones - I've made those changes and updated the CHANGELOG for this PR. (I'll squash the commits if it's all OK). Could you have another look over it? Cheers.

Copy link
Contributor

@MartinJJones MartinJJones left a comment

Choose a reason for hiding this comment

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

Nice work, changes look good to me 👍

@davidtrussler davidtrussler force-pushed the 2485_Refactor-layout-super-navigation-header-component branch from 89957f8 to fe7c847 Compare March 25, 2024 12:29
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3939 March 25, 2024 12:30 Inactive
@davidtrussler davidtrussler merged commit b183f1c into main Mar 25, 2024
13 checks passed
@davidtrussler davidtrussler deleted the 2485_Refactor-layout-super-navigation-header-component branch March 25, 2024 12:32
@andysellick andysellick mentioned this pull request Mar 25, 2024
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.

None yet

3 participants