CustomSelectControl: Add flag for larger default size #39401
Merged
Conversation
This file contains 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
mirka
force-pushed
the
enlarge-custom-select-control
branch
from
43befb2 to
ff4b0bd
Compare
15 tasks
|
Size Change: +139 B (0%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
|
Nice, thank you for the clear instructions. This is what I see in Storybook: Looks good, looks more coherent. Just for anyone else reviewing, we've had conversations about standardizing the heights across all inputs to be 36px, potentially with an option for 40px as well in the future. As we continue to refine those inputs and sizes, just unifying it all under a single 36px value is a good start, and will make future changes simpler. In that light, do we need to include On the padding, here's a screenshot of the resting state:
Here's one of the target designs: Here's a direct comparison of the metrics: A few small tweaks needed here. The left and right padding should be 16px (
There's a new chevronDownSmall icon, which we can use to better match the design, shown on the right here:
The color of the border (both for the dropdown and all input fields) should be #949494, or You note an alignment issue with the chevron needing a separate fix, sounds good, I don't think we need to do all of the above feedback in a single PR, but I figured I'd write it down regardless! Hope that's useful, thanks again! |
ciampo
reviewed
Mar 14, 2022
There was a problem hiding this comment.
Code changes LGTM, and thank you for refactoring the Story to use Controls!
I'll let you and @jasmussen discuss and refine the component's spacing!
In that light, do we need to include 36px in the flag at all? I.e. could it simply be called
__nextDefaultSize? While the current plan is to standardize all on 36px and have 40px as an option, we could go with__nextDefaultSizeand__nextLargeSizeinstead, and it'd be a bit more agnostic to the values.
I don't have a strong opinion here, both options sound good to me,
| 'components-custom-select-control__button', | ||
| { 'is-next-36px-default-size': __next36pxDefaultSize } | ||
| ), | ||
| isSmall: ! __next36pxDefaultSize, |
There was a problem hiding this comment.
Love it when we rely on composability instead of style overrides
|
Thanks for the reviews!
Good question. It isn't pretty, but the reason I prefer including a specific pixel value here is twofold:
Do you have any concerns about this diverging from the
Good to know. I've split out a separate issue (#39425) so we can change all of them at once. |
Excellent eye for detail. Thank you. It's my sense that we'd want to standardize on 16px padding left and right for all of these 36px items, including Buttons (barring any responsive changes we might make). That's perhaps the biggest benefit this effort has: unifying all these drifted values across a single grid. I'd think it okay to not do those changes all at once: in general we should avoid stacking dropdows and buttons vertically, and the places we do need love regardless. |
Gotcha, I've split out the As for this PR, I went ahead and changed it to the wider padding (126c3d9). I'll deal with the right side padding when I fix the chevron problem (#39400). With that, I think all feedback has been accounted for and this PR is ready for final approval 🙏 |
ciampo
approved these changes
Mar 18, 2022
There was a problem hiding this comment.
LGTM 🚀
- Horizontal padding has been adjusted to
16pxin this PR - Follow-ups:
- Chevron icon inconsistency tracked separately in CustomSelectControl: Make
chevronDownconsistent with SelectControl #39400 - Button padding inconsistency tracked separately in Button: Increase horizontal paddings to 16px #39431
- Border color inconsistencies tracked separately in Components: Lighten border colors to
gray-600#39425
- Chevron icon inconsistency tracked separately in CustomSelectControl: Make
jostnes
pushed a commit
to jostnes/gutenberg
that referenced
this pull request
Mar 23, 2022
* CustomSelectControl: Add flag for larger default size * Migrate stories * Add changelog entry * Increase paddings to 16px * Fixup changelog
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 an internal
__next36pxDefaultSizefeature flag prop to introduce the larger default size forCustomSelectControl.Why?
In preparation for standardizing the default sizes to 36px height.
How?
Following the procedure outlined in #39397, conditional styles have been added to enlarge the height and paddings. The font size is also enlarged to be consistent with
SelectControl.The inconsistent chevron (#39400) will be addressed separately.
@jasmussen How do the paddings feel?
Testing Instructions
npm run storybook:devand see the stories forCustomSelectControl.__next36pxDefaultSizeprop.Screenshots
__next36pxDefaultSize= false__next36pxDefaultSize= trueUpdated with 16px padding: