Style errors in selectControl prefix

[dabowman]

Description

The current state of the selectControl component allows for a prefix to be inserted. However, that prefix doesn't seem to respect the padding of the input as expected. The font size of the prefix also doesn't respond to screen size changes like the main input type style does.

Step-by-step reproduction instructions

You can see this in action in Storybook Add a value for prefix. See that there is no padding to the left (should be the same as what it was without a prefix). Also the spacing between the prefix and the text of the select should be 4px or .25rem.

You can then size the window down <600px to see the difference in font size between the prefix and the main slect text.

Screenshots, screen recording, code snippet

This is the correct design from the WordPress Design Library Figma file:

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[dabowman]

It looks like most of the styles, including the prefix styles are coming from the main input-control component. The portion of the component is getting an extra bit of type styling here:

https://github.com/WordPress/gutenberg/blob/0f37efd49b5f0b2effd65d273a06ca20337d986f/packages/components/src/select-control/style.scss#L7

We probably just need to add the prefix and suffix classes to this selector here to fix the type size issue.

dabowman commented 8 months ago

It also looks like the padding on the input needs to be adjusted if it has a prefix or suffix. Probably needs to be modified somewhere in here:

https://github.com/WordPress/gutenberg/blob/0f37efd49b5f0b2effd65d273a06ca20337d986f/packages/components/src/select-control/styles/select-control-styles.ts#L83

dabowman commented 8 months ago

I've got most of this fixed locally if someone can give me write access to push a branch?

dabowman commented 8 months ago

Made a PR to fix the type issue and the padding. The space between the prefix and the value still doesn't seem consistent but things are better. There are a lot more options on the component than there are in Figma, that's probably something that needs to get rectified in Figma so there's not any confusion when handing off to developers and designers are aware of the options and constraints on this component.

mirka commented 8 months ago

Hi! We provide a InputControlPrefixWrapper/InputControlSuffixWrapper utility component for folks who want responsive padding there. There's more information in the prefix/suffix prop descriptions in SelectControl, and also in the "With Prefix"/"With Suffix" stories for InputControl. We designed it this way because that padding is not wanted in some use cases.

As for the font sizes, I'm not sure it's universally desirable that the prefix font sizes increase responsively in a narrow viewport. We only increase the input font sizes there so we don't get the auto-zoom on iOS. There's no "inherent" reason to increase the prefix font size, and it may be undesirable in some cases due to limited viewport width. What do you think? If there is a strong need to match the font size in the prefix, we could consider rejiggering the styles a bit so the consumer can do a font-size: inherit when necessary.

dabowman commented 8 months ago

Yeah! I found the prefix and suffix wrapper components, which basically solve the padding issue. With that as an option it's fairly easy to add that padding if you want.

I think the question of what the default is is a good one. Currently, you need to do something on top of the component to get the prefix/suffix to look correct, whether that's using the provided component or modifying the css. Even when there's a good method provided, I don't love it when the component requires you to do something extra to effectively use a feature of it. That seems like a spot where folks would get confused and make mistakes. I think the question is whether the majority of implementations of the prefix/suffix would require padding or not. If folks are wanting this without padding most of the time then it makes sense to make that the default. However, I can't really think of a situation where you would want that unless you were making more style modifications on top of the component? Like, maybe if your prefix is an icon or something? or if you're changing the appearance of that text more and are adding, like a background to it or something? In all those cases you're probably writing more css, so I don't see why it would be bad to just write a bit more to get rid of that padding if you don't want it. Maybe y'all can fill me in more here. I'm not totally aware of how/where this is getting used.

At any rate it would be nice to document the prefix/suffix wrappers more in the selectControl component. The developers I've been working with initially added the correct padding with some css, which is sub-optimal if there's a nice component provided to do that for you. I don't think they knew it existed based on what's in storybook. The relationship between selectControl and inputControl aren't totally clear there and it is well documented in inputControl.

In my PR, I included adding the wrappers to the prefixes and suffixes in selectControl by default. I didn't add that to inputControl since I wasn't as familiar with how that was getting used. Again, interested to hear other opinions about whether or not it would be an issue to make that the default for selectControl.

As for the font size, I don't really see any scenario where you wouldn't want the prefix/suffix and input text size to be the same. At least, not at the sizes the input offers. By default, it looks like a mistake on smaller screens when the prefix/suffix text doesn't respond in the same way.

Conceivably, you could want to make them different sizes, but I don't think that's desirable from a design standpoint unless the implementation has some extra visual customization on top of it and in that case, you're customizing things already so it wouldn't be a problem to modify the type style from the default. I just think it makes more sense for the default to keep the design clean, even if folks might want to modify further on a case-by-case basis.

It seems like the approach to the defaults on some of these has been to do less and assume folks will make the last 10% of visual customizations themselves. Is that an accurate read? In some cases that might be desirable, but I do think everything should look not broken out of the box. There shouldn't be any reasonable permutations of properties on the components that cause visual errors, at least not on fundamental stuff like inputs.

Anways, interested to hear folks thoughts. Thanks for taking a look!

mirka commented 8 months ago

Currently, you need to do something on top of the component to get the prefix/suffix to look correct

I understand your perspective. There is a little more context given in the "Why?" section of #42378 — the main reason is back compat. Though, if I were to design this from scratch today I would probably opt for the same due to these two reasons:

• At least in our internal usages of the prefix/suffix props, many use cases specify paddings that are not the "standard" values. It hasn't been my impression that standard paddings are the clear majority. In a lot of cases, people want a little bit tighter.
• Padding is additive, so it's kind of unintuitive to have to add a utility wrapper component to subtract/reset that padding.

In any case, reversing the padding on SelectControl is a breaking change and we can't do that without a very strong reason.

it would be nice to document the prefix/suffix wrappers more in the selectControl component.

Agreed!

As for the font size, I don't really see any scenario where you wouldn't want the prefix/suffix and input text size to be the same.

I don't have a strong opinion about this, but I think this change can be considered a bug fix and not a breaking change. So it's something we can address.

Action items

• [x] Add "With Prefix" and "With Suffix" stories to the Storybook for SelectControl, similar to InputControl. #65730
• [ ] Set prefix/suffix to be the same font-size as the input. (We need to be careful about how/where to do the styling so it isn't overly specific. We don't want to override anyone who already has a custom font-size set.)

Does that sound reasonable?

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[dabowman]

It also looks like the padding on the input needs to be adjusted if it has a prefix or suffix. Probably needs to be modified somewhere in here:

https://github.com/WordPress/gutenberg/blob/0f37efd49b5f0b2effd65d273a06ca20337d986f/packages/components/src/select-control/styles/select-control-styles.ts#L83

[dabowman]

I've got most of this fixed locally if someone can give me write access to push a branch?

[dabowman]

Made a PR to fix the type issue and the padding. The space between the prefix and the value still doesn't seem consistent but things are better. There are a lot more options on the component than there are in Figma, that's probably something that needs to get rectified in Figma so there's not any confusion when handing off to developers and designers are aware of the options and constraints on this component.

[mirka]

Hi! We provide a InputControlPrefixWrapper/InputControlSuffixWrapper utility component for folks who want responsive padding there. There's more information in the prefix/suffix prop descriptions in SelectControl, and also in the "With Prefix"/"With Suffix" stories for InputControl. We designed it this way because that padding is not wanted in some use cases.

As for the font sizes, I'm not sure it's universally desirable that the prefix font sizes increase responsively in a narrow viewport. We only increase the input font sizes there so we don't get the auto-zoom on iOS. There's no "inherent" reason to increase the prefix font size, and it may be undesirable in some cases due to limited viewport width. What do you think? If there is a strong need to match the font size in the prefix, we could consider rejiggering the styles a bit so the consumer can do a font-size: inherit when necessary.

[dabowman]

Yeah! I found the prefix and suffix wrapper components, which basically solve the padding issue. With that as an option it's fairly easy to add that padding if you want.

I think the question of what the default is is a good one. Currently, you need to do something on top of the component to get the prefix/suffix to look correct, whether that's using the provided component or modifying the css. Even when there's a good method provided, I don't love it when the component requires you to do something extra to effectively use a feature of it. That seems like a spot where folks would get confused and make mistakes. I think the question is whether the majority of implementations of the prefix/suffix would require padding or not. If folks are wanting this without padding most of the time then it makes sense to make that the default. However, I can't really think of a situation where you would want that unless you were making more style modifications on top of the component? Like, maybe if your prefix is an icon or something? or if you're changing the appearance of that text more and are adding, like a background to it or something? In all those cases you're probably writing more css, so I don't see why it would be bad to just write a bit more to get rid of that padding if you don't want it. Maybe y'all can fill me in more here. I'm not totally aware of how/where this is getting used.

At any rate it would be nice to document the prefix/suffix wrappers more in the selectControl component. The developers I've been working with initially added the correct padding with some css, which is sub-optimal if there's a nice component provided to do that for you. I don't think they knew it existed based on what's in storybook. The relationship between selectControl and inputControl aren't totally clear there and it is well documented in inputControl.

In my PR, I included adding the wrappers to the prefixes and suffixes in selectControl by default. I didn't add that to inputControl since I wasn't as familiar with how that was getting used. Again, interested to hear other opinions about whether or not it would be an issue to make that the default for selectControl.

As for the font size, I don't really see any scenario where you wouldn't want the prefix/suffix and input text size to be the same. At least, not at the sizes the input offers. By default, it looks like a mistake on smaller screens when the prefix/suffix text doesn't respond in the same way.

Conceivably, you could want to make them different sizes, but I don't think that's desirable from a design standpoint unless the implementation has some extra visual customization on top of it and in that case, you're customizing things already so it wouldn't be a problem to modify the type style from the default. I just think it makes more sense for the default to keep the design clean, even if folks might want to modify further on a case-by-case basis.

It seems like the approach to the defaults on some of these has been to do less and assume folks will make the last 10% of visual customizations themselves. Is that an accurate read? In some cases that might be desirable, but I do think everything should look not broken out of the box. There shouldn't be any reasonable permutations of properties on the components that cause visual errors, at least not on fundamental stuff like inputs.

Anways, interested to hear folks thoughts. Thanks for taking a look!

[mirka]

Currently, you need to do something on top of the component to get the prefix/suffix to look correct

I understand your perspective. There is a little more context given in the "Why?" section of #42378 — the main reason is back compat. Though, if I were to design this from scratch today I would probably opt for the same due to these two reasons:

• At least in our internal usages of the prefix/suffix props, many use cases specify paddings that are not the "standard" values. It hasn't been my impression that standard paddings are the clear majority. In a lot of cases, people want a little bit tighter.
• Padding is additive, so it's kind of unintuitive to have to add a utility wrapper component to subtract/reset that padding.

In any case, reversing the padding on SelectControl is a breaking change and we can't do that without a very strong reason.

it would be nice to document the prefix/suffix wrappers more in the selectControl component.

Agreed!

As for the font size, I don't really see any scenario where you wouldn't want the prefix/suffix and input text size to be the same.

I don't have a strong opinion about this, but I think this change can be considered a bug fix and not a breaking change. So it's something we can address.

Action items

• [x] Add "With Prefix" and "With Suffix" stories to the Storybook for SelectControl, similar to InputControl. #65730
• [ ] Set prefix/suffix to be the same font-size as the input. (We need to be careful about how/where to do the styling so it isn't overly specific. We don't want to override anyone who already has a custom font-size set.)

Does that sound reasonable?