ComboboxControl / FormTokenField: Add flag for larger default size #40746
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
15 tasks
|
Size Change: +191 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
ciampo
reviewed
May 2, 2022
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
I appreciate this PR is still a draft, but I left a few early comments nonetheless :)
| const deprecatedDefaultSize = ( { __next36pxDefaultSize } ) => | ||
| ! __next36pxDefaultSize && | ||
| css` | ||
| height: 30px; |
Contributor
There was a problem hiding this comment.
Just to understand — prior to the changes from this PR, is ComboBox height "computed" from the sum of its intrinsic input height + paddings/borders ?
Member
Author
There was a problem hiding this comment.
Yes, and it was wrong 😅 In trunk the height is 29px instead of 30px.
Usually I don't like hardcoding input heights in pixels because it's hostile to users who have a custom font size in set in their browsers, but this repo codes all font sizes in pixel values instead of rems, so... it makes no difference 🙃 Perhaps that ship has sailed.
mirka
force-pushed
the
form-token-field-combobox-36px
branch
from
9d6552f to
2514e59
Compare
mirka
force-pushed
the
form-token-field-combobox-36px
branch
from
2514e59 to
3cd6112
Compare
mirka
commented
Jun 17, 2022
| const deprecatedDefaultSize = ( { __next36pxDefaultSize } ) => | ||
| ! __next36pxDefaultSize && | ||
| css` | ||
| height: 30px; |
Member
Author
There was a problem hiding this comment.
Yes, and it was wrong 😅 In trunk the height is 29px instead of 30px.
Usually I don't like hardcoding input heights in pixels because it's hostile to users who have a custom font size in set in their browsers, but this repo codes all font sizes in pixel values instead of rems, so... it makes no difference 🙃 Perhaps that ship has sailed.
| .components-form-token-field__token { | ||
| font-size: $default-font-size; | ||
| display: flex; | ||
| margin: 2px 4px 2px 0; |
Member
Author
There was a problem hiding this comment.
Instead of handling the spacing between tokens with margins here, we now use a flex gap on the container (<TokensAndInputWrapper>). This way the token margins don't mess with the container's paddings.
| align-items: center; | ||
| width: 100%; | ||
| margin: 0 0 $grid-unit-10 0; | ||
| padding: $grid-unit-05*0.5 $grid-unit-05; |
Member
Author
There was a problem hiding this comment.
We're refactoring this padding so it's handled by a subcontainer that doesn't contain the suggestions list (<TokensAndInputWrapper>). This allows us to remove the awkward negative margins on .components-form-token-field__suggestions-list that were needed to break out of that padding.
Comment on lines
+17
to
+25
| const deprecatedPaddings = ( { | ||
| __next36pxDefaultSize, | ||
| hasTokens, | ||
| }: TokensAndInputWrapperProps ) => | ||
| ! __next36pxDefaultSize && | ||
| css` | ||
| padding-top: ${ space( hasTokens ? 1 : 0.5 ) }; | ||
| padding-bottom: ${ space( hasTokens ? 1 : 0.5 ) }; | ||
| `; |
Member
Author
There was a problem hiding this comment.
In trunk, the input height increases a bit when you commit a token. I'm keeping this behavior for now in the deprecated case, but in the 36px case there won't be anymore height jumps.
Contributor
There was a problem hiding this comment.
Sounds like a sensible approach
ciampo
reviewed
Jun 21, 2022
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Great work here!
I noticed that the new version of the Combobox is taller than its version on trunk. The element wrapping the input has an overall height of 32px (30px height, plus 1px of border top and bottom)
combobox-height.mp4
Is this expected? For example, I had a look at InputControl, which seems to have an overall height of 30px (and uses an absolutely-positioned backdrop to render a border).
Similarly, in the 36px version of the component, the overall input's height is effectively 38px once we consider the border.
Comment on lines
27
to
31
| export const TokensAndInputWrapper = styled.div` | ||
| display: flex; | ||
| flex-wrap: wrap; | ||
| align-items: center; | ||
| gap: ${ space( 1 ) }; |
Contributor
There was a problem hiding this comment.
Is there any reason why you prefer CSS styles to using the Flex and HStack components?
Member
Author
There was a problem hiding this comment.
Lol I forgot that existed! Replaced in 7e411f1.
I noticed that <Flex> does some magical margin computations under the hood to replicate gap in older browsers. Maybe we can remove that soon now that Safari is compliant.
Comment on lines
+17
to
+25
| const deprecatedPaddings = ( { | ||
| __next36pxDefaultSize, | ||
| hasTokens, | ||
| }: TokensAndInputWrapperProps ) => | ||
| ! __next36pxDefaultSize && | ||
| css` | ||
| padding-top: ${ space( hasTokens ? 1 : 0.5 ) }; | ||
| padding-bottom: ${ space( hasTokens ? 1 : 0.5 ) }; | ||
| `; |
Contributor
There was a problem hiding this comment.
Sounds like a sensible approach
Member
Author
Totally forgot to fix that, thanks for the catch! |
Member
Author
|
Just to note: Although we're pausing active work on #39397, I'd like to land this PR since the work is already done, and it includes some nice element refactoring that might also help us when adding the 40px variants. |
ciampo
approved these changes
Jul 1, 2022
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
- ✅ All feedback addressed
- ✅ Input fields have the correct height (
30px,36px) - ✅ Tests well in Storybook
🚀
mburridge
added
the
Needs Dev Note
Contributor
|
Added the Needs Dev Note label in case this needs a dev note (either individual or as part of a "misc" dev note) for WP 6.1 release. |
Contributor
|
@mirka probably we should write a general dev note for the margin work that you've been doing? |
Member
Author
|
Yes, I've been tagging my margin PRs that involve actual deprecations, so we'll definitely have a Dev Note about those. For this specific PR though, |
mirka
removed
the
Needs Dev Note
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #39397
What?
Adds the temporary prop
__next36pxDefaultSizeto opt into the new 36px height default size.Why?
As part of the effort to move toward consistent component sizes.
This PR had to include two components at once because
ComboboxControldepends on someFormTokenFieldsubcomponents internally.How?
See inline comments for notable points.
Testing Instructions
npm run storybook:devComboboxControlandFormTokenField. They should look nice with the__next36pxDefaultSizeprop enabled. With the prop disabled, it should look the same as in trunk (ComboboxControl, FormTokenField).FormTokenField, also test the case where there are a lot of tokens and it wraps to the next line.Known issues
I imagine we'll want to make the paddings in the suggestion list roomier to match the larger input size, but I'll leave that to a future PR.