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

Fix homepage super navigation buttons when text scale is increased #4232

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

AshGDS
Copy link
Contributor

@AshGDS AshGDS commented Sep 17, 2024

What / Why

Visual Changes

Before

image image

After

image image

https://trello.com/c/2BSs27tS/35-homepage-visual-issue-when-font-scale-is-increased-on-mobile

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4232 September 17, 2024 14:27 Inactive
@AshGDS AshGDS self-assigned this Sep 17, 2024
@AshGDS AshGDS marked this pull request as ready for review September 17, 2024 14:38
@AshGDS AshGDS force-pushed the menu-fix-text-scale branch from 7b6a770 to 91c8b3a Compare September 18, 2024 09:50
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4232 September 18, 2024 09:50 Inactive
Copy link
Contributor

@jon-kirwan jon-kirwan left a comment

Choose a reason for hiding this comment

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

It looks good to me and have approved (I just added one small suggestion around the ampersand selector).

@@ -606,6 +608,10 @@ $after-button-padding-left: govuk-spacing(4);
// Styles for search toggle button.
.gem-c-layout-super-navigation-header__search-toggle-button {
background: none;

&--blue-background {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a small suggestion - I don't think we have any style guidelines on this, but these types of selectors can be a bit tricky to search for. We could change it to gem-c-layout-super-navigation-header__search-toggle-button--blue-background though I realise that might be a bit verbose so no worries if not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jon-kirwan - good suggestion, will do this and then merge.

@AshGDS AshGDS force-pushed the menu-fix-text-scale branch from 91c8b3a to 243da63 Compare September 18, 2024 12:38
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4232 September 18, 2024 12:38 Inactive
@AshGDS AshGDS force-pushed the menu-fix-text-scale branch from 243da63 to 2e6c00d Compare September 18, 2024 12:39
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4232 September 18, 2024 12:39 Inactive
@AshGDS AshGDS merged commit 59bc3f3 into main Sep 18, 2024
12 checks passed
@AshGDS AshGDS deleted the menu-fix-text-scale branch September 18, 2024 12:45
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.

Super navigation header visual bug on mobile when text scale is increased
3 participants