Components: Update the TextControl padding to match the rest of the controls #64326
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
added
[Type] Bug
|
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. |
youknowriad
force-pushed
the
update/text-control-padding
branch
from
9f68024 to
90bac52
Compare
|
This seems to create some inconsistency in different UIs (block padding/margin controls, border radius and border...) I wonder if the right fix is the opposite (use the smaller padding for all controls). |
|
Size Change: +7 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
|
This makes the values consistent which is a win, but the padding looks a bit too large to me.
Conditionally approving, since changing everything would likely need to be done separately. |
jameskoster
approved these changes
Aug 7, 2024
tyxla
reviewed
Aug 7, 2024
mirka
approved these changes
Aug 7, 2024
Comment on lines
23
to
24
| padding-inline-start: $grid-unit-20 - 1px; | ||
| padding-inline-end: $grid-unit-20 - 1px; |
There was a problem hiding this comment.
Subtracting 1px to account for the fact that the newer components do not include a border on the underlying element. Also tweaked to use logical properties (though it doesn't really matter here), since we'll likely start linting for that in this package.
Temporary story diff for testing:
diff --git a/packages/components/src/input-control/stories/index.story.tsx b/packages/components/src/input-control/stories/index.story.tsx
index 6067c467f9..0b3353e0c7 100644
--- a/packages/components/src/input-control/stories/index.story.tsx
+++ b/packages/components/src/input-control/stories/index.story.tsx
@@ -14,6 +14,9 @@ import InputControl from '..';
import { InputControlPrefixWrapper } from '../input-prefix-wrapper';
import { InputControlSuffixWrapper } from '../input-suffix-wrapper';
import Button from '../../button';
+import TextControl from '../../text-control';
+import SelectControl from '../../select-control';
+import { VStack } from '../../v-stack';
const meta: Meta< typeof InputControl > = {
title: 'Components (Experimental)/InputControl',
@@ -112,3 +115,23 @@ export const ShowPassword: StoryFn< typeof InputControl > = ( args ) => {
/>
);
};
+
+export const SizeTest = {
+ render: () => {
+ return (
+ <VStack spacing={ 2 }>
+ <InputControl __next40pxDefaultSize value="Hello" />
+ <TextControl
+ __nextHasNoMarginBottom
+ __next40pxDefaultSize
+ value="Hello"
+ onChange={ () => null }
+ />
+ <SelectControl
+ __next40pxDefaultSize
+ options={ [ { label: 'Hello', value: 'hello' } ] }
+ />
+ </VStack>
+ );
+ },
+};
There was a problem hiding this comment.
What's the difference between InputControl and TextControl?
There was a problem hiding this comment.
Should we add a comment to explain the need for subtracting 1px?
There was a problem hiding this comment.
What's the difference between
InputControlandTextControl?
InputControl is newer, still formally experimental, and has a few more features that TextControl doesn't - like prefix, suffix, and additional ways to display the label. Check the storybook stories to get a better idea.
There was a problem hiding this comment.
InputControl is newer, still formally experimental, and has a few more features that TextControl doesn't - like prefix, suffix, and additional ways to display the label. Check the storybook stories to get a better idea.
Why didn't we just update TextControl with the new features (and new implementation)
There was a problem hiding this comment.
I wasn't part of the team back then, but IIRC InputControl came along with G2, and was aiming to be TextControl's successor. My assumption is that we preferred a new component in order to supply all the new features (including making it themeable) without breaking any backwards compatibility. It was also intended for those 2 components to converge, but it never happened. It might be something to pursue at some point, and it's possible that others might have thought about it already (@WordPress/gutenberg-components)
There was a problem hiding this comment.
The converging could've happened as part of #59418, but due to the Emotion performance issues we found in #58948 (comment), I'd want to wait until we remove Emotion from InputControl. It is definitely in the plans though, since it doesn't make sense to have two input components in the package.
youknowriad
force-pushed
the
update/text-control-padding
branch
from
decf231 to
96382a5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related #57394
What?
I noticed that the TextControl component has a different padding than Number and Select controls (when used with the 40px size). This PR tries to fix that.
I'm not entirely sure that this PR fixes all the cases raised in the issue above, it only focuses on TextControl.
Why?
Consistency of the design system.
Testing Instructions
npm run storybook:dev