Skip to content
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

NavigatorButton: Reuse Button types #47754

Merged
merged 3 commits into from
Feb 7, 2023
Merged

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Feb 4, 2023

What?

Part of #35744

Following up #46997 , this PR cleans up types in NavigatorButton by re-using types from the Button component

Why?

Clean up duplicated code

How?

Button's types are complex, since the component can render as a <button /> or as a <a /> depending on which props are passed.

Since NavigatorButton uses Button with the assumption that it renders a <button /> (and never a <a />), I decided to export a ButtonPropsAsButton type and reuse it in NavigatorButton.

Testing Instructions

There aren't any runtime changes, so everything should work as before if the project builds without TS errors.

@ciampo ciampo added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Feb 4, 2023
@ciampo ciampo requested review from mirka and kienstra February 4, 2023 10:17
@ciampo ciampo self-assigned this Feb 4, 2023
@ciampo ciampo requested a review from ajitbohra as a code owner February 4, 2023 10:17
@ciampo
Copy link
Contributor Author

ciampo commented Feb 4, 2023

For now I went with the simplest / laziest approach, but here's a few more considerations.

When working on NavigatorButton, we decided to use Button (instead of a vanilla HTML <button />) as the underlying component, for visual coherency. We also made the component polymorphic, in case consumers wanted to BYOB(utton).

On one hand, it feels correct that we advertise all of ButtonPropsAsButton props for the NavigatorButton component, so that consumers can leverage all of Button's customisation options.

On the other hand, could exposing all the props be problematic if, e.g, the consumer decides to render NavigatorButton as any other element/component ? All those ButtonPropsAsButton props would not really make sense — and any non-optional prop could also become annoying, since TS would require it even if it didn't made sense with the as prop.

Maybe an option could be to re-use all props of ButtonPropsAsButton, but intentionally marking them as optional ?

Any thoughts?

@github-actions
Copy link

github-actions bot commented Feb 4, 2023

Flaky tests detected in ceb13a1.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4114034401
📝 Reported issues:

Copy link
Contributor

@kienstra kienstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ciampo,
Good idea to refer to the <Button> props for when it's a <button>.

*/
children: ReactNode;
};
export type NavigatorBackButtonProps = ButtonAsButtonProps;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to simplify this!

@ciampo ciampo force-pushed the refactor/navigator-use-button-types branch from 10e08ed to ceb13a1 Compare February 7, 2023 12:47
@ciampo ciampo enabled auto-merge (squash) February 7, 2023 12:47
@ciampo ciampo merged commit 2ce5507 into trunk Feb 7, 2023
@ciampo ciampo deleted the refactor/navigator-use-button-types branch February 7, 2023 13:15
@github-actions github-actions bot added this to the Gutenberg 15.2 milestone Feb 7, 2023
@ciampo ciampo added Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Feb 7, 2023
ntsekouras pushed a commit that referenced this pull request Feb 9, 2023
* `NavigatorButton`: Reuse `Button` types

* CHANGELOG

* Tweak README
@ntsekouras
Copy link
Contributor

Cherry-picked this PR to the wp/6.2 branch.

@ntsekouras ntsekouras removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants