Add __unstable-large size variant on InputControl SelectControl UnitControl#35646
Merged
Conversation
mirka
added
the
[Feature] UI Components
|
Size Change: +67 B (0%) Total Size: 1.1 MB
ℹ️ View Unchanged
|
mirka
force-pushed
the
enlarge-input-controls
branch
from
52664bb to
d4d5bef
Compare
2 tasks
mirka
force-pushed
the
enlarge-input-controls
branch
from
d4d5bef to
27ef7a9
Compare
mirka
force-pushed
the
storybook/lazyload-css
branch
from
502cc4c to
96fc690
Compare
Maintains previous size for the `small` variant.
There is a default marginBottom on the Spacer component that throws off the vertical alignment here.
The `small` variant is now more appropriate in this use case.
To prevent unnecessary truncation in mobile-width viewports when the font size gets bigger.
Maintains previous size for the `small` variant.
- Simplifies unit alignment across sizes - Fixes paddings for disableUnits
mirka
force-pushed
the
enlarge-input-controls
branch
from
27ef7a9 to
a05caf7
Compare
mirka
changed the title
default variantdefault variant
mirka
changed the title
default variantdefault variant
Compensates for wider padding on InputControl
mirka
changed the title
default variantdefault variant
mirka
changed the title
default variantdefault variant
mirka
force-pushed
the
enlarge-input-controls
branch
from
6dcd9fc to
345da0b
Compare
mirka
changed the title
default variant__unstable-large size variant on InputControl SelectControl UnitControl
Member
Author
|
I've scoped this down to be a preparatory PR — non-breaking and basically no visual changes. The PR description has been updated, and includes a list of next steps to selectively enable these large sizes. @jasmussen Does this plan sound good to you? |
7 tasks
Contributor
|
I do like the sound of "no visual changes", especially if it allows you to move forward. I suggested a bit of a crazy idea in your other PR, not sure if that's helpful here also. |
Contributor
I was of the understanding that all the separate PRs were to be merged together. This would effectively set all the controls to the |
aaronrobertshaw
left a comment
aaronrobertshaw
left a comment
Contributor
There was a problem hiding this comment.
I've given this another test after the change in approach.
Functionally everything is working however I did find some visual changes in terms of the height of some controls. In particular, the unit control. It appears that the tweaks to it have increased its height.
| Before | After |
|---|---|
 |
 |
 |
 |
 |
 |
Comment on lines
+147
to
+149
| height: 36, | ||
| lineHeight: 1, | ||
| minHeight: 30, | ||
| minHeight: 36, |
Contributor
There was a problem hiding this comment.
I might be misinterpreting these changes here but weren't the default sizes to remain unchanged?
After some initial feedback and reconsideration, this PR now simply prepares an __unstable-large size variant on several basic components to allow opt-in upsizing in specific contexts. Default sizes are unchanged.
Contributor
There was a problem hiding this comment.
I think we're moving towards having two sizes, 36px default, and 40px in some cases. At the moment, some of them are 30, some of them are 32, so it really is a bit all over the place, and so it feels okay to update some of these even if it does make for a few pixels of change here and there. For example most buttons are 36px, such as "Move to trash", which makes it really stand out next to a 30px input field.
In the other PR I wondered if we could use variables for these. We could set all values for both small and large to the most generic current value, such as 32. Then once we have a slew of these components updated, we can update both to 36 as a new baseline, and then one by one upgrade those that need it (notably in global styles and tools panels) to 40.
Contributor
There was a problem hiding this comment.
At the moment, some of them are 30, some of them are 32, so it really is a bit all over the place, and so it feels okay to update some of these even if it does make for a few pixels of change here and there.
Ok, I'm just confused. If it's alright that some controls are temporarily a few pixels out why couldn't they be a few pixels out and be set to 40px straight away, thereby reducing the work and time to achieve the end result?
Contributor
There was a problem hiding this comment.
Totally valid. In my opinion, 2-6px is a smallish change, and notably 36px would bring them in line with existing button sizes.
40px is a pretty big change, and a bold layout that only works when all counterparts are the same size.
But you're right though, if we can't touch controls like text-decoration toggle groups at the same time, we should definitely strive for "no visual changes" until such a time as we can do them all at the same time. But I'm unsure: could we use a variable to help us here?
Contributor
There was a problem hiding this comment.
I'm not sure if a variable is feasible here given some components may apply the heights via hard coded styles. @mirka will be able to give a more informed opinion on that.
As for controls like the text-decoration toggle groups, I'd see that just being another PR in the "batch". When they are all in place and ready, they could all be merged. If we wanted to test them all together we could apply the patches for each PR on a single branch and confirm everything lines up nicely before hitting merge on anything.
Given all of Lena's hard work so far, I'd be inclined to support whatever approach she's most comfortable with.
Member
Author
There was a problem hiding this comment.
I might be misinterpreting these changes here but weren't the default sizes to remain unchanged?
Argh great catch, you saved my life there! That was unintended, just a mangled revert — I confused it with the 36px default for button-like things. (I guess that's how confusing these size variants are, even after focusing on it for the past few days 😅)
Fixed now.
7 tasks
Contributor
|
After the recent updates, everything looks good to me in both the block editor and storybook examples. I didn't spot any visual regressions compared to
|
# Conflicts: # packages/components/CHANGELOG.md
ciampo
added
[Package] Components
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 #36230
Description
Previous iteration of this PR
Increases the size of the
defaultInputControl to the new design (40px height with 16px horizontal paddings).As part of this PR, we will audit all components and make the minimum necessary tweaks on those that depend on InputControl, so they do not look broken.
Modified components (see commits for specifics)
Aside from the components that were directly modified, the following components are also affected and checked for breakage:
input-controlmixin (e.g. TextControl, CustomSelectControl, ComboboxControl, etc.) will be dealt with in a separate PR.How has this been tested?
defaultandsmallvariants look goodAfter some initial feedback and reconsideration, this PR now simply prepares an
__unstable-largesize variant on several basic components to allow opt-in upsizing in specific contexts. Default sizes are unchanged.Breaking changes have been eliminated, and there are no actual visual changes being enabled in this PR. (Aside from a minor padding inconsistency fix in
UnitControl.)Changes
RangeControl🔧 tweaked internally (accommodated Emotion class changes)🐛 corrected inconsistent padding
🔧 tweaked internally (simplified unit positioning to adapt to different heights)
Next steps (separate PRs)
How has this been tested?
InputControl,SelectControl, andUnitControltested in Storybook.Remaining tasks
Types of changes
New feature / Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).