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

Center text and icons vertically in the option select component #4256

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

Conversation

matthillco
Copy link
Contributor

@matthillco matthillco commented Sep 25, 2024

What

Vertically centre align the toggle icons with the label in Option Selects. Trello

Why

The font size of this element increased with the typography changes deployed in a recent update to govuk-frontend. This means that the up/down icons no longer align vertically. This PR vertically centres the icons with the text for better vertical balance. A new container for the icon and button is also required so that the selected counter element can be styled independently.

This also supports the same change made in finder-frontend where search filter titles had the same tweak to icon positions. See the PR at alphagov/finder-frontend#3334.

Visual Changes

Before After
image image

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4256 September 25, 2024 08:16 Inactive
@matthillco matthillco force-pushed the option-select-title-vertical-center branch from 09177bf to 3857241 Compare September 25, 2024 08:19
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4256 September 25, 2024 08:19 Inactive
@matthillco matthillco changed the title Center text and icons vertically in the option select component [WIP] Center text and icons vertically in the option select component Sep 26, 2024
@matthillco matthillco force-pushed the option-select-title-vertical-center branch from 3857241 to bdf6355 Compare September 30, 2024 15:16
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4256 September 30, 2024 15:16 Inactive
@matthillco matthillco force-pushed the option-select-title-vertical-center branch from bdf6355 to bebabea Compare October 2, 2024 11:52
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4256 October 2, 2024 11:52 Inactive
@matthillco matthillco force-pushed the option-select-title-vertical-center branch from bebabea to ab06156 Compare October 2, 2024 15:14
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4256 October 2, 2024 15:14 Inactive
@matthillco matthillco force-pushed the option-select-title-vertical-center branch from ab06156 to 9defca8 Compare October 2, 2024 15:22
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4256 October 2, 2024 15:22 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.

Changes look good to me overall, and the approach seems consistent with those made to the expander component in finder-frontend.

Just one thing which is likely worth looking at further in relation to the icon size.

@@ -76,11 +76,9 @@

.gem-c-option-select__icon {
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing that could happen when switching to use flex for the layout is that the icon will shrink depending on the length of the text, for example:
Screenshot 2024-10-03 at 09 27 12

One possible option to stop the icon from shrinking is to add flex: 0 0 30px. I imagine the same CSS rule would then need to be added to the expander component in finder-frontend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good spot, thanks! I've added a flex-shrink: 0 to solve this rather than your suggestion, as it avoids the need to update the number if the size of the icon changes. @MartinJJones

@matthillco matthillco force-pushed the option-select-title-vertical-center branch from 9defca8 to 31b6df3 Compare October 3, 2024 15:09
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-4256 October 3, 2024 15:10 Inactive
The font size of this element increased with the typography changes
deployed in a recent update to `govuk-frontend`. This means that the
up/down icons no longer align vertically. This PR centers the icons
with the text for better vertical balance. A new container for the
icon and button is also required so that the selected counter element
can be styled independently.

This also supports the same change made in `finder-frontend` where
search filter titles had the same tweak to icon positions. See the PR
at alphagov/finder-frontend#3334.
@matthillco matthillco force-pushed the option-select-title-vertical-center branch from 31b6df3 to c4d5065 Compare October 3, 2024 16:01
@matthillco matthillco changed the title [WIP] Center text and icons vertically in the option select component Center text and icons vertically in the option select component Oct 3, 2024
@matthillco
Copy link
Contributor Author

Changes look good to me overall, and the approach seems consistent with those made to the expander component in finder-frontend.

Just one thing which is likely worth looking at further in relation to the icon size.

Thanks @MartinJJones , I've made the fix you suggested, would you be able to take a look and approve? Many thanks.

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