InputControl: Decrease large default padding if has prefix/suffix by mirka · Pull Request #42166 · WordPress/gutenberg

mirka · 2022-07-05T17:53:54Z

What?

Ensures that the padding between the prefix/suffix and the text input doesn't get too big in the large size variant of the component.

Why?

The large size variant of the component increases the side paddings of the text input to 16px (previously 8px). In most cases, this is too big when using with a prefix or suffix element. Some actual examples of this can be seen in the large variants of UnitControl and ColorPicker:

How?

Added an internal API to specify custom side padding on the InputField subcomponent. This API is internal-only for now, and is meant to minimally satisfy the internal customization needs in the components library. Though, I tried to make it generic enough so it would be easy to expose the API externally on InputControl itself, if we want to do so in the future.

Testing Instructions

npm run storybook:dev
Check the "With Prefix" and "With Suffix" stories for the InputControl component. The padding between the prefix/suffix and the text input should remain at 8px, regardless of the size variant. This should also work in RTL mode.
Check the stories for UnitControl. There should be no regression in padding between the text input and the unit dropdown.

Screenshots or screencast

Before	After

mirka · 2022-07-05T17:54:24Z

+	prefix: <span style={ { marginInlineStart: 8 } }>@</span>,
 };

 export const WithSuffix = Template.bind( {} );
 WithSuffix.args = {
 	...Default.args,
-	suffix: <button style={ { marginRight: 4 } }>Send</button>,
+	suffix: <button style={ { marginInlineEnd: 4 } }>Send</button>,


Tweaked examples for better RTL.

mirka · 2022-07-05T17:54:48Z


 type InputProps = {
 	disableUnits?: boolean;
-	size: SelectSize;


Unrelated, but minor cleanup of an unused type.

mirka · 2022-07-05T17:59:58Z

-const paddingStyles = ( { disableUnits }: InputProps ) => {
-	if ( disableUnits ) return '';
-
-	return css`
-		${ rtl( { paddingRight: 8 } )() };
-	`;
-};
-


CSS override removed 🎉

Btw @aaronrobertshaw: When building BorderControl, do you remember if there were any blockers to rendering the BorderControlDropdown as a prefix on the UnitControl? I'm wondering if we can remove some CSS overrides in there too if this PR lands.

My memory is failing me on this one, I don't recall any blockers. It might have been as simple as I didn't see the prefix prop in the UnitControl readme and didn't follow the props through to the underlying InputControl.

It also looks like we might need to tweak the UnitControl types, as attempting to pass the border control dropdown via a prefix prop on the UnitControl results in a type error that it expects prefix to be string | undefined.

I'd be happy to work on rendering the border control's dropdown as a prefix in a follow up to this PR.

That sounds great, thanks! It's nice to see things getting less hacky as we converge on common patterns 🙂

I've put together a PR (#42212) that now renders the dropdown via the UnitControl prefix and attempts to clean up the styles. There could still be more to polish on that control when it comes time to add a large 40px size variant.

ciampo

I just left a couple of comments related more to the implementation details of these changes, but the overall direction of the PR looks good, and the changes test well in Storybook!

aaronrobertshaw

This is looking good and tests well for me 👍

✅ With Prefix/Suffix stories maintain 8px padding regardless of size variant
✅ Padding functions consistently when in RTL mode
✅ No regressions for UnitControl in Storybook
✅ UnitControl tests pass

jameskoster · 2022-07-06T09:14:19Z

Seems to be working as expected, though ideally we should increase the margin/padding on the prefix/suffix as well, so that it matches the input:

mirka · 2022-07-06T19:50:55Z

ideally we should increase the margin/padding on the prefix/suffix as well

Currently InputControl doesn't add any left padding to the prefix, instead leaving it to the prefix element itself to handle it. I believe this is because sometimes the prefix/suffix elements need to be flush with the text field.

But I agree that it's kind of necessary that the prefix/suffix elements know what the standard padding is for a given size variant. As a follow-up task, I'll see if I can propose something where InputControl provides the prefix component with a paddingInlineStart prop so the prefix can use that padding value for styling if they want.

Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>

mirka · 2022-07-12T21:01:06Z

But I agree that it's kind of necessary that the prefix/suffix elements know what the standard padding is for a given size variant. As a follow-up task, I'll see if I can propose something where InputControl provides the prefix component with a paddingInlineStart prop so the prefix can use that padding value for styling if they want.

Solution proposed in #42378 👍

This PR can proceed independently of that PR though.

ciampo

Given previous conversations and the follow-up already opened in #42378, I believe we can go ahead and merge this one 🚀

mirka added 4 commits July 6, 2022 02:17

InputControl: Decrease padding when prefix/suffix

ef42dbf

UnitControl: Remove padding override

3b0e3ec

UnitControl: Clean up unused type

1a98f98

Tweak story examples for better RTL support

8ddc126

mirka added the [Package] Components /packages/components label Jul 5, 2022

mirka self-assigned this Jul 5, 2022

mirka requested a review from ajitbohra as a code owner July 5, 2022 17:53

mirka commented Jul 5, 2022

View reviewed changes

mirka requested review from aaronrobertshaw, chad1008 and ciampo July 5, 2022 18:01

Add changelog

edf295b

mirka mentioned this pull request Jul 5, 2022

ColorPicker: Cleanly implement large size #42002

Merged

Update snapshot

dc3e9ca

ciampo reviewed Jul 5, 2022

View reviewed changes

Comment thread packages/components/src/input-control/styles/input-control-styles.tsx

Comment thread packages/components/src/input-control/index.tsx

Add comment

38069ad

aaronrobertshaw reviewed Jul 6, 2022

View reviewed changes

Comment thread packages/components/src/input-control/styles/input-control-styles.tsx Outdated

Fix typo

8329445

Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>

mirka requested a review from ciampo July 12, 2022 01:49

mirka mentioned this pull request Jul 12, 2022

InputControl: Add padding wrapper for prefix/suffix #42378

Merged

ciampo added [Type] Enhancement A suggestion for improvement. [Feature] UI Components Impacts or related to the UI component system labels Jul 14, 2022

ciampo approved these changes Jul 14, 2022

View reviewed changes

mirka merged commit a979537 into trunk Jul 14, 2022

mirka deleted the fix/input-prefix-suffix-padding branch July 14, 2022 22:25

github-actions Bot added this to the Gutenberg 13.8 milestone Jul 14, 2022

Conversation

mirka commented Jul 5, 2022

What?

Why?

How?

Testing Instructions

Screenshots or screencast

Uh oh!

mirka Jul 5, 2022

Choose a reason for hiding this comment

Uh oh!

mirka Jul 5, 2022

Choose a reason for hiding this comment

Uh oh!

mirka Jul 5, 2022

Choose a reason for hiding this comment

Uh oh!

aaronrobertshaw Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

mirka Jul 6, 2022

Choose a reason for hiding this comment

Uh oh!

aaronrobertshaw Jul 7, 2022

Choose a reason for hiding this comment

Uh oh!

ciampo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aaronrobertshaw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jameskoster commented Jul 6, 2022

Uh oh!

mirka commented Jul 6, 2022

Uh oh!

mirka commented Jul 12, 2022

Uh oh!

ciampo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants