Skip to content

Composite: accept store props on top level component#64832

Merged
ciampo merged 2 commits into
trunkfrom
feat/stabilize-composite/accept-store
Sep 2, 2024
Merged

Composite: accept store props on top level component#64832
ciampo merged 2 commits into
trunkfrom
feat/stabilize-composite/accept-store

Conversation

@ciampo
Copy link
Copy Markdown
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
@github-actions
Copy link
Copy Markdown

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.

@github-actions
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@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