-
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
InputControl: Add padding wrapper for prefix/suffix #42378
Conversation
Size Change: +188 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
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'm not sure if naming props like exact CSS properties (i.e. paddingLeft
and paddingRight
) is a good idea — I usually prefer prop names that hide the implementation details.
Also, I'm not sure if there's a point in allowing consumers of the component to define a custom padding; I can imagine that 99.9% of the time, the requirements are either to have the default spacing, or to have no spacing at all.
With this in mind, I propose an alternative approach:
- the
InputControlPrefix
andInputControlSuffix
have ahasSpacing
boolean prop (an alternative name could behasInlineEndSpacing
for the prefix andhasInlineStartSpacing
for the suffix) - prop's default value TBD
- when the prop is
false
, no padding is added - when the prop is
true
, the padding corresponding to theInputControl
's size is added (via internal styles)
WDYT?
That makes sense. However, when I tried to implement it, I realized that we can't accomplish what we want here using the Context System if we don't expose the But that prompted me to think of an alternative, simpler way, which is to remove
Sound good? |
An alternative way to the context system, maybe could have been to:
Out of curiosity, do you think that would work?
Sounds like a good solution, yes. Maybe we should call the components |
Without the context system, we'd need the consumer to explicitly pass a
Makes sense. Done in c697074 👍 |
Tests well in Storybook, although I noticed that we're using Should we use logic CSS properties in this PR too? |
This PR uses the |
Follow-up to #42166
What?
Adds a convenience wrapper component
InputControlPrefix
/InputControlSuffix
that can be used by a consumer to add standard padding that matches the size variant.Wrapped prefix when InputControl is at default size
Wrapped prefix when InputControl is at large size
(The padding between the prefix and the placeholder here is too wide — this will be addressed in #42166)
Why?
Currently, whenever a consumer wants to add padding before their
prefix
element, they need to add it themselves. This is not sustainable because they need to know the padding value of the underlyingInputControl
, and that value also changes depending on the size variant.Here are the alternative approaches I considered, but decided against:
<CardMedia>
approach taken in the Card component, and the approach considered forDropdown
. Although it would be nice to have a similar construct forInputControl
, the current implementation is already no padding by default. Also, it's arguably more natural in this case for the wrapper to add padding instead of break out of it, since we're only dealing with one side of the box. (A padding breakout wrapper deals with all four sides of the box.)prefix
is a function, and if it is, call it with a{ paddingInlineStart }
argument so the consumer can use that value to style their own margin. This seems simple on the surface, but it's just more work for the consumer, and forces them to use an inline style or a CSS-in-JS solution. Also, I think the general direction we're headed toward for these solutions is to use subcomponents likeCardMedia
.How?
Provide a thin wrapper component that applies responsive padding through the Context System. This can be used with any InputControl-based component that inherits the
prefix
/suffix
props.Testing Instructions
npm run storybook:dev
InputControl
.