-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add variants to InputControl prefix/suffix wrappers #64824
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
const { paddingLeft, paddingRight } = getSizeConfig( { | ||
inputSize: size, | ||
__next40pxDefaultSize, | ||
} ); | ||
const prefixSuffixContextValue = useMemo( () => { | ||
return { | ||
InputControlPrefixWrapper: { paddingLeft: `${ paddingLeft }px` }, | ||
InputControlSuffixWrapper: { paddingRight: `${ paddingRight }px` }, | ||
InputControlPrefixWrapper: { __next40pxDefaultSize, size }, | ||
InputControlSuffixWrapper: { __next40pxDefaultSize, size }, | ||
}; | ||
}, [ paddingLeft, paddingRight ] ); |
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.
Moving all the style logic into the styles file.
Flaky tests detected in 2633c4c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10578497331
|
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 🚀
Apart from UnitControl
, do you see other places where we may use an explicit variant? I thought SearchControl
but then I remembered that we probably never got to refactor it to use InputControl
internally
* Internal prop used to control the padding size of the wrapper. | ||
* | ||
* @ignore | ||
*/ | ||
children: ReactNode; | ||
size?: BaseProps[ 'size' ]; | ||
/** | ||
* Internal prop used to control the padding size of the wrapper. | ||
* | ||
* @ignore | ||
*/ | ||
__next40pxDefaultSize?: BaseProps[ '__next40pxDefaultSize' ]; |
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.
Is @ignore
-ing a prop and writing "internal" in the description enough in your opinion to allow us to make future changes to it?
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.
Yes. Also in this case I do consider these props to be pretty much part of the API, just set through context by InputBase
for convenience.
It is refactored actually 😄 In the components package, these wrapper variants can potentially be used for UnitControl (unit select), NumberControl (custom steppers), SearchControl (search icon), SelectControl (caret), CustomSelectControl (caret), etc. Not all of them currently use a suffix wrapper for their suffix content, so it would likely remove some ad hoc CSS if replaced. Not a priority though, since they aren't currently broken or anything. In my mind this is more for consumer usage, so they can easily implement prefixes/suffixes like this. |
Makes sense. Thank you for the explanation! |
* Add variants to InputControl prefix/suffix wrappers * Use smaller close icon * Add changelog * Update snapshot Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org>
Prerequisite for remaining UnitControl migrations in #63871
What?
Adds a
variant
prop toInputControlPrefixWrapper
andInputControlSuffixWrapper
components to better handle icons and buttons.Why?
We currently have to add custom CSS to decrease paddings when the prefix/suffix content is an icon or control (e.g. button). This should be easier, as it is a common use case.
Currently, the
icon
andcontrol
variant have the same styles, but I kept them separate variants for future proofing.Testing Instructions
Screenshots or screencast
Paddings adhere to the current specs.
Default size
Compact size