-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
UnitControl: Add flag for larger default size #40627
Conversation
const paddingStyles = ( { disableUnits }: InputProps ) => { | ||
if ( disableUnits ) return ''; | ||
|
||
return css` | ||
${ rtl( paddings[ size ] )() }; | ||
${ rtl( { paddingRight: 8 } )() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simplified the logic here to capture the intent better. Basically, we want the padding-right to always be 8px when units are enabled. Otherwise, it simply inherits from the paddings that InputControl
specifies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this, so much easier to read!
Not for this PR, but I wonder if we should specify these amounts using the space utility (i.e. multiples of 4
) — we should probably do an overhaul of the config before that, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
const paddingStyles = ( { disableUnits }: InputProps ) => { | ||
if ( disableUnits ) return ''; | ||
|
||
return css` | ||
${ rtl( paddings[ size ] )() }; | ||
${ rtl( { paddingRight: 8 } )() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this, so much easier to read!
Not for this PR, but I wonder if we should specify these amounts using the space utility (i.e. multiples of 4
) — we should probably do an overhaul of the config before that, though.
cf37ecf
to
695b9f2
Compare
Part of #39397
What?
Adds the temporary prop
__next36pxDefaultSize
to opt into the new 36px height default size.Why?
As part of the effort to move toward consistent component sizes.
How?
Details of the migration plan are written in the original issue #39397.
Testing Instructions
npm run storybook:dev
UnitControl
. By setting the__next36pxDefaultSize
control totrue
, the height of thedefault
size variant should increase to 36px. All other size variants are unchanged.Screenshots or screencast
Old default height
New default height (opt-in for now)