-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Button : deprecate isSmall prop
#59734
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
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. |
mirka
left a comment
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 was a bit concerned about the existing deprecation patterns we have in this component, so I'm proposing #59913 to set better conventions for the future. Could you take a look at that please? Then whatever we agree on there, we can integrate those patterns back into this PR.
| deprecated( 'Button isSmall prop', { | ||
| since: '6.6', | ||
| alternative: 'size="small"', | ||
| version: '6.9', |
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.
| version: '6.9', |
I'm thinking we don't really have to do a hard deprecation on this one, since the maintenance cost is very low. What do you think?
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 think we should start following those more actively and actually clean them all up when they're due. With that in mind, I find them useful and I think they help reduce technical debt over time. scratch that, this comment explained that really well.
|
I updated this PR based on #59913. Perhaps the deprecated warning itself is unnecessary? |
mirka
left a comment
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, thank you!
| }; | ||
|
|
||
| if ( isSmall ) { | ||
| deprecated( 'Button isSmall prop', { |
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.
Perhaps the deprecated warning itself is unnecessary?
Yes, I personally don't think it's worth it in this particular case, but I'm not going to block it if you or others think it's warranted. I will suggest adding a Dev Note for the next WP release though.
If we're going to throw a warning, we should add a prefix:
| deprecated( 'Button isSmall prop', { | |
| deprecated( 'wp.components.Button isSmall prop', { |
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 decided to remove the deprecated() function for now so it doesn't throw any warnings. In the future, we may need to consider adding warnings along with other props such as isPrimary|isTertiary|isSecondary|isLink.
* Button : deprecate `isSmall` prop * Update changelog * Update types * revert unit test changes * Remove version prop from deprecated function * Remove `deprecated()` functon Co-authored-by: t-hamano <[email protected]> Co-authored-by: mirka <[email protected]> Co-authored-by: tyxla <[email protected]>
Closes #59733
What?
This PR officially deprecates the
isSmallprop of theButtoncomponent.Why?
#51842 introduced
sizeprop, which coversisSmallprop. #53560 also replaced theisSmallprop with thesizeprop forButtoncomponents throughout the Guenberg project. This should allow us to officially deprecate theisSmallprop.How?
I added an error indicating that a deprecation warning appears from WP6.6 and will be removed in WP6.9.At the same time, I updated types and unit tests.Testing Instructions
All CIs should be green. Additionally, use the code below to confirm that a console error is displayed when a
Buttoncomponent with theisSmallprop is registered as a custom block.