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.

Sign up for GitHub

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

CustomSelectControlV2: fix item styles #62825

Merged
merged 7 commits into from
Jul 1, 2024
Merged

CustomSelectControlV2: fix item styles #62825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0d8c8a1
Fix line height
ciampo Jun 25, 2024
f04567f
Add disabled styles
ciampo Jun 25, 2024
eccd3cd
CHANGELOG
ciampo Jun 25, 2024
ad808c5
Prevent user selection for items text
ciampo Jun 25, 2024
0820b99
Apply more tweaks
ciampo Jun 26, 2024
1ec1750
Simplify line-height styles
ciampo Jul 1, 2024
ee95320
Less opinionated item disabled styles
ciampo Jul 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
### Internal

- `CustomSelectControlV2`: prevent keyboard event propagation in legacy wrapper. ([#62907](https://github.com/WordPress/gutenberg/pull/62907))
- `CustomSelectControlV2`: fix item styles ([#62825](https://github.com/WordPress/gutenberg/pull/62825))
- `CustomSelectControlV2`: add root element wrapper. ([#62803](https://github.com/WordPress/gutenberg/pull/62803))
- `CustomSelectControlV2`: fix popover styles. ([#62821](https://github.com/WordPress/gutenberg/pull/62821))
- `CustomSelectControlV2`: fix trigger text alignment in RTL languages ([#62869](https://github.com/WordPress/gutenberg/pull/62869)).
Expand Down
11 changes: 10 additions & 1 deletion packages/components/src/custom-select-control-v2/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ export const Select = styled( Ariakit.Select, {
font-family: inherit;
font-size: ${ CONFIG.fontSize };
text-align: start;
user-select: none;
width: 100%;

&[data-focus-visible] {
Expand Down Expand Up @@ -133,7 +134,15 @@ export const SelectItem = styled( Ariakit.SelectItem )`
justify-content: space-between;
padding: ${ ITEM_PADDING };
font-size: ${ CONFIG.fontSize };
line-height: 2.15rem; // TODO: Remove this in default but keep for back-compat in legacy
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To match 28px, the line height value should have been 2.15 (without the rem unit). I felt like a more explicit calc syntax would help to understand the reason for an arbitrary number.

Copy link
Member

Choose a reason for hiding this comment

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

It's curious where the 2.15 came from originally...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the ratio of the line height (28px) to the font size (13px)

Also, since 28 / 13 = 2.15384615, another consequence of using 2.15 is that the resulting line height wouldn't be exactly 28px (as per design), but slightly less because of the lower decimal precision.

In short: I prefer to let the browser (or sass) do the calculation, and if not possible, to state the line-height directly.

Copy link
Member

Choose a reason for hiding this comment

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

Also, since 28 / 13 = 2.15384615, another consequence of using 2.15 is that the resulting line height wouldn't be exactly 28px (as per design), but slightly less because of the lower decimal precision.

Which also makes me question if 28/13 was the intent. But yes, having a clear calc there makes things much clearer.

// TODO: reassess line-height for non-legacy v2
line-height: 28px;
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer 2.15384615 and adding a comment about how it's calculated, because that way we'll keep the flexibility/relativity in case someone modifies only the font-size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #62825 (comment)

scroll-margin: ${ space( 1 ) };
user-select: none;

&[aria-disabled='true'] {
cursor: not-allowed;
}

&[data-active-item] {
background-color: ${ COLORS.theme.gray[ 300 ] };
}
Expand Down
Loading