You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or 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
This PR fixes the right padding when the button has text and an icon positioned on the right. This is required to compensate the extra padding that the icon adds. This is already working for the icon positioned on the left, but the right position of the icon was not taken into account.
Why?
Because when the button has text and an icon positioned on the right, there is more padding on the right of the button than on the left.
How?
Adding an additional css class named has-icon-right that is only added when the icon is positioned on the right. That class will apply the correct padding in the css file.
Testing Instructions
Apply this patch and run storybook by executing npm run storybook:dev
Once storybook opens, finds a button component
Select primary, so it's easier to see the background
On the Controls tab on the bottom, find the icon prop and select an icon, e.g. wordpress
On the iconPosition prop below, select right
Alternatively, you can add the following queryParams to the URL ?path=/story/components-button--primary&args=icon:wordpress;iconPosition:right
Check the left and right padding on the bottom are similar
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 props-bot label.
If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @epeicher! In case you missed it, we'd love to have you join us in our Slack community.
If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.
The reason will be displayed to describe this comment to others. Learn more.
I wondered if there might be a way to style this without an additional class†, but I think this is a simple solution consistent with the existing style arrangement for left icon + text. LGTM 👍
† The original styles prior to #50277 targeted svg elements, so technically we could do something like .button:has(svg:last-child) { ... }, but targeting svg broadly doesn't feel like it provides as much guarantees as here with icon and icon positioning.
I wondered if there might be a way to style this without an additional class†, but I think this is a simple solution consistent with the existing style arrangement for left icon + text. LGTM 👍
† The original styles prior to #50277 targeted svg elements, so technically we could do something like .button:has(svg:last-child) { ... }, but targeting svg broadly doesn't feel like it provides as much guarantees as here with icon and icon positioning.
Thanks @aduth for the review! I agree with this, it is a simple solution consistent with the existing code, and targeting svg broadly could be more fragile
It looks like the failing Unit Tests are true failures pointing to some snapshots that need to be updated. Can you update those?
Thanks for letting me know! I didn't realise of that, I have updated the snapshots by running npm run test:unit:update and now running npm run test:unit works locally for me 👍
Thanks for the updates! I had to fight a bit with some merge conflicts and flakey tests after adding a changelog note, but finally got this merged today 😄
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.
github-actions
bot
added
the
First-time Contributor
t-hamano
added
[Type] Bug
What?
Closes #60734
This PR fixes the right padding when the button has text and an icon positioned on the right. This is required to compensate the extra padding that the icon adds. This is already working for the icon positioned on the left, but the right position of the icon was not taken into account.
Why?
Because when the button has text and an icon positioned on the right, there is more padding on the right of the button than on the left.
How?
Adding an additional
cssclass namedhas-icon-rightthat is only added when the icon is positioned on the right. That class will apply the correct padding in thecssfile.Testing Instructions
npm run storybook:devControlstab on the bottom, find theiconprop and select an icon, e.g.wordpressiconPositionprop below, selectright?path=/story/components-button--primary&args=icon:wordpress;iconPosition:rightTesting Instructions for Keyboard
Screenshots or screencast