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

Tidy unused CSS in layout super navigation component #4228

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

csutter
Copy link
Contributor

@csutter csutter commented Sep 16, 2024

Tip

This PR moves a lot of code around blocks and is best viewed with whitespace changes ignored (?w=1)

What

Removes some cruft from the layout super navigation header component.

Why

Removing code sparks joy. Also as a stepping stone to help us change the search menu to a new design in a future PR.

Visual Changes

None.

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4228 September 16, 2024 14:28 Inactive
@csutter csutter changed the title Super navigation header: Small cleanup Tidy unused CSS in layout super navigation component Sep 16, 2024
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4228 September 16, 2024 14:49 Inactive
- Remove unused styles from previous iterations of the markup that
  haven't been removed
- Remove unnecessarily convoluted selector targetting element
  `.gem-c-layout-super-navigation-header__navigation-top-toggle-button`
  both with and without `--blue-background` modifier
- Move open state logic into main `top-toggle-button` block
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4228 September 16, 2024 14:51 Inactive
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.

The removal of the unused CSS looks good to me, good spot!

I spotted one small thing I with one of the changes, just let me know if I can help further on it 👍

}

.gem-c-layout-super-navigation-header__navigation-top-toggle-button.gem-c-layout-super-navigation-header__navigation-top-toggle-button--blue-background,
.gem-c-layout-super-navigation-header__navigation-top-toggle-button {
Copy link
Contributor

Choose a reason for hiding this comment

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

It does make sense to remove this to me, but when testing the blue background variation of the component on Safari for iOS it will cause the "Menu" text for the button to appear white instead of blue when clicked, for example:

iOS-menu-example

Likely several ways to solve this, possibly re-ordering the CSS to help set the expected precedence to avoid adding further CSS. This could take a bit of work and likely out of scope for the original intended purpose of code cleanup in the PR.

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.

3 participants