Button: Try improving padding for icon + text buttons. #46764
Merged
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
jasmussen
added
[Feature] UI Components
|
Size Change: +4 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
jorgefilipecosta
approved these changes
Jan 3, 2023
|
Nice PR. Appreciate it's not directly part of this work, but does the Dashicon affordance really need a custom off-grid margin value? Should we make that 8px too? |
mirka
approved these changes
Jan 4, 2023
There was a problem hiding this comment.
I haven't been able to test, however it seems like this would break in RTL contexts.
FYI, we have an RTL switcher tool right inside Storybook for testing this 🙂
CleanShot.2023-01-05.at.04.24.14.mp4
Furthermore, the button component appears to have an iconPosition prop, which at the moment seems to do nothing — even if set to right, the icon still shows up on the left
The iconPosition prop currently only works when the text is provided via the text prop and not as children, like this.
The Button component is a mess right now to begin with (#44042) so I'm ok with merging incremental fixes (i.e. half-measures) as long as it doesn't mess things up further. This PR doesn't fix the icon+text case, but it fixes the icon+children case and doesn't break anything else. So I think we're good to go once we have a changelog entry 👍
The footprint of dashicons are 20x20 vs 24x24, so the margin has to be plus the extra pixels to vertically align if you mix and match. I do think there's a place where we retire the dashicons support here, but that's separate. |
jasmussen
force-pushed
the
try/improve-icon-button-padding
branch
from
7d18804 to
fedc155
Compare
|
Flaky tests detected in fedc155. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3910685952
|
|
I'll land this for now, but will happily follow up with any edits in case something was off. Thanks again folks! |
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.
What?
Primary and secondary Buttons have an awkward padding when both an icon and text is provided. You can see this in Storybook, or in this PR: #46014:
By default, the button has 6px top/bottom padding, 12px left/right padding. But when a button
has-icon, that padding becomes a blanket 6px:This PR fixes it, by providing a 12px right padding, baked in:
Adding this padding, at a glance, seems safe enough since the code a few lines down applies the following:
That is, the component already assumes a left to right reading direction, with the icon on the left.
I haven't been able to test, however it seems like this would break in RTL contexts. Furthermore, the button component appears to have an
iconPositionprop, which at the moment seems to do nothing — even if set to right, the icon still shows up on the left: https://wordpress.github.io/gutenberg/?path=/story/components-button--default&args=icon:more;iconPosition:right;variant:primaryAs is, this PR will improve things for primary and secondary buttons, and does not tackle the problem of RTL or iconPosition being broken. However it would be good to follow up on those.
Testing Instructions
Perhaps easiest to test in storybook.
https://wordpress.github.io/gutenberg/?path=/story/components-button--default&args=icon:more;iconPosition:right;variant:primary