-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Button: Place children before the icon when iconPosition
is "right"
#59489
Conversation
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. |
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.
Change looks good 🚀
Bit weird how TextProp
and TextChildren
smoosh into each other, but that's outside the scope of this PR!
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.
Looks good, thanks!
Bit weird how TextProp and TextChildren smoosh into each other, but that's outside the scope of this PR!
Yeah, but I guess this is limited to this example. I'd say that if we attempt to fix this, it might actually break, or unexpectedly alter other existing usages that use either only children or text.
Thanks for the PR!
Based on my experience, I expect that there are almost no cases where the |
Should be able to close #44042
What?
This PR renders children in front of the icon in the
Button
component wheniconPosition
is set toright
. This change will always render the icon last if bothchildren
,text
are used andiconPositon
isright
.Example code:
Before
After
Why?
In #44042, when the
Button
component has an icon, some issues were reported regarding the spacing and ordering between the icon and text.These issues have been fixed in stages as a result of the following PRs.
gap
is applied if the button has an icon: Update has-text has-icon button spacing #50277has-text
class is now applied not only whenchildren
is present, but also when thetext
prop is present: Components: Fix logic ofhas-text
class addition in Button #56949However, the only problem still unresolved is that the icon is rendered before the
children
, even though theiconPosition
isright
.How?
Simply swapped the order in which
children
is rendered. This change will result in a visual change for consumers usingchildren
,text
,icon
,iconPosition="right"
. However, there are probably no users who use both children and text and expect an icon to appear between them.Note
As discussed in #56949, it seems that deprecation of the
text
prop is also being considered.Testing Instructions
text
prop and apply an icon via the control panel.iconPosition
prop to ensure that it is always rendered to the left or right.Screenshots or screencast
a709dc82413570b845c8dde54ed9f311.mp4