Refactor/toolbar button component to typescript

[mike-day]

Part of #35744.

To be merged after #47087.

What?

Convert the ToolbarButton component to TypeScript.

Why?

This PR is needed as part of an effort to convert Toolbar and related Toolbar subcomponents to TypeScript.

How?

By adding types to ToolbarButton and ToolbarButtonContainer.

Testing Instructions

1. Create or otherwise open a post
2. Add a block that would have some toolbar controls (e.g. Image)
3. Expected: Toolbar buttons should fundamentally work the same as before:

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @mike-day! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[mike-day]

@ciampo @mirka here is some initial work for ToolbarButton, intended to be merged after the refactoring of Toolbar to TypeScript is merged.

[mike-day]

Note that commit and file change history will be much smaller before this is merged — #47087 was pulled in to avoid duplicate work.

[ciampo]

Hey @mike-day , we haven't taken a look at this PR as it's still marked as a draft — just let us know when and we'll take a look!

[mike-day]

I am not sure about the purpose of extraProps — both props and extraProps are spread into Button.

[mirka]

Agreed. The props passing seems fishy overall, especially with how props.foo is used for some things. I have to guess that this code has degraded over time.

I'm also not entirely convinced that we should be accepting all Button props, since some of them are probably not appropriate in this specific toolbar context. But perhaps we can revisit this later when there is a more pressing need to improve these typings and APIs.

For posterity, I'll also note that I'm fine with resorting to an any type here for the time being. Ideally it should be some variation of ButtonProps, but that forces us to deal with the button/anchor polymorphism to an extent that I'd rather do it as a separate issue. I kind of feel like we should only allow "Button as a <button>" inside toolbars, but even that decision requires some more investigation and effort!

[ciampo]

This actually relates well to the work that @chad1008 is doing on CircularOptionPicker's TypeScript refactor. For now, we've decided to use ButtonAsButtonProps (so that we can avoid the anchor case), but I'd definitely be interested in your opinion once the PR is ready for review

[mike-day]

@ciampo @mirka this has been left as any for now. Let me know if a different approach should be taken.

Also, I believe I have worked through the suggestions left prior. This should be ready for review again.

[mike-day]

Hey @mike-day , we haven't taken a look at this PR as it's still marked as a draft — just let us know when and we'll take a look!

Checks are running from a fresh pull of trunk and resolving of CHANGELOG, but this should be ready for review, @ciampo!

I opted to not rebase and leave commit history alone, but just let me know if I should clean that up (pulling in 47087 in the beginning of this work made commit history look more extensive than it really is).

[mirka]

Looks good! This has uncovered some existing bugs and API issues, but those will be triaged and dealt with separately 👍

[mirka]

Agreed. The props passing seems fishy overall, especially with how props.foo is used for some things. I have to guess that this code has degraded over time.

I'm also not entirely convinced that we should be accepting all Button props, since some of them are probably not appropriate in this specific toolbar context. But perhaps we can revisit this later when there is a more pressing need to improve these typings and APIs.

For posterity, I'll also note that I'm fine with resorting to an any type here for the time being. Ideally it should be some variation of ButtonProps, but that forces us to deal with the button/anchor polymorphism to an extent that I'd rather do it as a separate issue. I kind of feel like we should only allow "Button as a <button>" inside toolbars, but even that decision requires some more investigation and effort!

[ciampo]

Hey @mike-day , do you still have capacity to work on this PR?

[mike-day]

Hey @mike-day , do you still have capacity to work on this PR?

Yes, apologies for the radio silence! I plan to jump back on this and work through comments tomorrow.

[mirka]

Great work, thank you!

I just went ahead and moved back the changelog into the current "Unreleased" section 👍

[ciampo]

I believe this is the first time that I see us using props from another React element (Button) as the second argument for the WordPressComponentProps — @mirka do you see any potential issue with this?

[mirka]

Good question. Considering this from several angles:

• Can Emotion handle polymorphism like this? — Yes. The Emotion as prop allows React.ElementType, so it can potentially handle as={ Button } if this was a polymorphic WordPressComponent.
• Is it a good idea to allow something like as={ Button }? — In general, I don't like it because it's hard to predict what will happen. But it may make sense in some abstract components.
• Should we prefer ToolbarButtonProps & ButtonProps syntax in cases like this? — No strong opinion, given that there are no serious downsides to the WordPressComponentProps syntax. I like how we can just do typeof Button without importing the ButtonProps type though.

[ciampo]

Good observations. Let's keep as-is, keep an eye for any unintended consequences, and otherwise consider it as another tool in our belt