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

Composite: accept store props on top level component #64832

Merged
merged 2 commits into from
Sep 2, 2024

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Aug 27, 2024

What?

Extracted from #64723

Tweak Composite so that store props can be also passed to the top-level Composite component.

Why?

This is the first step towards removing explicit usage of the Composite store. All props that were previously passed to the useCompositeStore hook can be now passed to the top-level component instead.

This PR is the requirement for all upcoming refactors.

How?

By changing the type defs and reconciliating existing store objects.

Testing Instructions

Make sure that existing usages of Composite continue to work.

@ciampo ciampo force-pushed the feat/stabilize-composite/accept-store branch 2 times, most recently from 1b9270c to 4c941ea Compare August 27, 2024 14:33
@ciampo ciampo self-assigned this Aug 27, 2024
@ciampo ciampo added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Aug 27, 2024
@ciampo ciampo force-pushed the feat/stabilize-composite/accept-store branch from 4c941ea to f454e85 Compare August 27, 2024 20:51
@ciampo ciampo force-pushed the feat/stabilize-composite/accept-store branch from f454e85 to 2f1e4e3 Compare August 31, 2024 09:18
@ciampo ciampo marked this pull request as ready for review August 31, 2024 09:20
@ciampo ciampo requested a review from ajitbohra as a code owner August 31, 2024 09:20
@ciampo ciampo requested a review from a team August 31, 2024 09:20
Copy link

github-actions bot commented Aug 31, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link

Flaky tests detected in fb1db16.
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/10644161549
📝 Reported issues:

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Tested in the inserter and in data views, all works well.

🚀

rtl,
} );

const store = storeProp || newStore;
Copy link
Member

Choose a reason for hiding this comment

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

Any good reason to prefer || to ?? here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Main reasons is that I copied the same style as the ariakit repo when dealing with store fallbacks:

https://github.com/ariakit/ariakit/blob/main/packages/ariakit-react-core/src/composite/composite.tsx#L145-L146

Also, store shouldn't have falsy values ('', 0) which would cause || to behave differently than ??

@ciampo ciampo merged commit c7a991c into trunk Sep 2, 2024
63 of 65 checks passed
@ciampo ciampo deleted the feat/stabilize-composite/accept-store branch September 2, 2024 09:07
@github-actions github-actions bot added this to the Gutenberg 19.2 milestone Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants