Components: Replace Button with ui/Button using deprecated props strategy #29990
Conversation
sarayourfriend
added
[Package] Components
sarayourfriend
requested review from
ajitbohra and
chrisvanpatten
as code owners
There was a problem hiding this comment.
This is a great PR because it allows us to reason precisely about the difference in APIs and make informed decisions about whether we want to deprecate things or not.
I'll note that I think that "an API without compromises" doesn't exist, what we might think is perfect today, might not be tomorrow, for this reason, sometimes, it's fine to avoid introducing a new API even if admittedly it's better than the existing one but if it's merely a proxy or renaming of existing prop, I might ask why bother at all. This is me saying that aside "this is a better name for the prop or a better format" we might want to think about whether the tradeoff to rename or change the format of a prop is worth the deprecation or if it's it fine just to keep the old API.
That said, this is the exact kind of PR I was looking to see, we'll be able to discuss precisely the migration path. I can't review deeply now, I'll circle back later.
| deprecated( 'Button icon as string', { | ||
| alternative: 'Button icon as ReactElement', | ||
| hint: 'Use the icons from @wordpress/icons', | ||
| } ); |
There was a problem hiding this comment.
I know for instance that this is not something we can't officially deprecate yet. Blocks can use string icons and they can't rely on svg or packages in a lot of situations.
There was a problem hiding this comment.
Okay! In that case we can just remove the deprecation warning, right?
|
Size Change: -838 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
@sarayourfriend I think this is a great approach! The "breakdown" of the individual deprecated props provides a replicable strategy for handling remapping/deprecation/(removal?) of props :). I think Running Gutenberg locally, we can see the UI breaking in many areas 🙈 .
It's mostly style based. However, I think this reveals a lot of awkward / sub-optimal uses of A downside of this strategy is that we can't incrementally scope and roll-out the new implementation - via the protection of the Context System. I think it's worth noting that there's a difference between rendering a UI with |
|
Thanks for the screenshot @ItsJonQ. That really illustrates one of the significant difficulties of this migration process. Like you said, the approach taken in this PR doesn't allow for piecemeal, area-by-area adaption. The new component must work for all instances. I think that's a fine expectation if we take this approach, but obviously more work needs to be done on |
|
I upgraded G2 and it fixes some of the problems but not all. Upgrading G2 gets us the new low specificity. |
I echo what @youknowriad mentioned. I see that public APIs between the old and new version of the button differs a lot. I would personally look at this from a plugin developer perspective that might suddenly see a big number of warnings coming from components they use in the project. There isn't any immediate benefit for them to start using the latest API because the button will work more or less the same. It's important to be aware of that side of the components adoption and ensure that it doesn't become too troublesome and folks will lose confidence in using WordPress components. |
|
A couple questions for @ItsJonQ regarding this PR. I think this PR exposes something about G2's Button that was not talked about yet. Basically, it is too different from the existing Button. Aside from API improvements, are there aspects of the G2 Button that are "must haves" that the current Button does not implement? My thought behind asking this question is: can we actually just use the existing button, improve it in some ways (including by refining the API to use a Are there also other components we can do this with? I'm going to open two new PRs that are inspired by this one. One for Text and one for Flex. I think these components in general can actually use this current strategy and get to a mergable place soon. |
Description
Replaces the old Button's API surface with a new API surface that combines the deprecated props + the new button's props and then ultimately transforms all of them into the new button, with deprecation warnings, then renders the new button from
ui/button.This is an experimental PR as a proof of concept as an alternative to the
ComponentSystemProviderstrategy we're currently following.This could be preferrable because it exposes the new API directly without forcing the old API to be a mask for the new one. You can continue to use the deprecated API with the deprecation warnings. Alternatively, you can use the new API and receive no deprecation warnings, but both are available.
Note: All the button tests will fail as they're expecting the old button implementation. We should just remove the tests and write tests for the new hook if we follow this strategy. Tests on the
ui/buttoncomponent should suffice.Renamed props:
isPressedisActiveisBusyisLoadingI'm not sold on
isLoadingbeing better thanisBusy.isBusyhas a wider meaning that I think we should prefer and preserve instead of deprecating it in favor ofisLoading.isActiveI think is better thanisPressedthough, for the same reason whyisBusyis better thanisLoading.isActivehas a wider meaning that doesn't reference a specific type of interaction. CurrentlyisPressedis used to indicate when the complimentary area is open, whileisActiveis probably a more semantic name.Part of #29689
How has this been tested?
Storybook (
npm run storybook:dev) buttons continue to work as expected.Screenshots
Types of changes
Non-breaking deprecation changes.
Checklist: