-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Conversation
1b9270c
to
4c941ea
Compare
4c941ea
to
f454e85
Compare
f454e85
to
2f1e4e3
Compare
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Flaky tests detected in fb1db16. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10644161549
|
There was a problem hiding this 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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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:
Also, store
shouldn't have falsy values (''
, 0
) which would cause ||
to behave differently than ??
What?
Extracted from #64723
Tweak
Composite
so that store props can be also passed to the top-levelComposite
component.Why?
This is the first step towards removing explicit usage of the
Composite
store. All props that were previously passed to theuseCompositeStore
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.