DisclosureContent: migrate from reakit to @ariakit/react
#55639
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
flootr
added
[Type] Code Quality
9 tasks
|
Size Change: -408 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
flootr
force-pushed
the
feat/disclosure-content-ariakit
branch
from
9e2732a
to
d9aed65
Compare
flootr
requested review from
mirka,
ciampo,
tyxla,
chad1008,
brookewp and
andrewhayward
brookewp
reviewed
Nov 2, 2023
tyxla
previously requested changes
Nov 3, 2023
There was a problem hiding this comment.
I have some concerns regarding keeping the same APIs as before, and whether it's a good idea, considering that the Ariakit API already supports all cases we're adding. I left a few comments to outline those.
In general, I wonder if we should just deprecate the old version and just expose a v2 that solely relies on the Ariakit Disclosure component family?
| * Accessible Disclosure component that controls visibility of a section of | ||
| * content. It follows the WAI-ARIA Disclosure Pattern. | ||
| * | ||
| * @see https://ariakit.org/reference/disclosure-content |
There was a problem hiding this comment.
I wonder if we should also reference some more examples that include how the entire disclosure component family works (with the provider, disclosure, and disclosure content). We could link to the examples in the Ariakit docs of course, but it could be useful for someone who needs to understand the whole picture of the Disclosure component family.
There was a problem hiding this comment.
In the light of the approach chosen for this PR, it's probably better not to link to ariakit docs, since DisclosureContent wouldn't currently have the same public APIs as the ariakit component.
|
Hey @tyxla , let me add some extra context around these changes. Markus and I recently discussed how to go about these changes. Markus proposed to have a first PR (this one) where he would try to refactor from If you think that it's better to compound those changes into one PR, I'm happy to go for that approach and to start understanding the best way forward. |
I just thought this step was unnecessary since we'll end up either removing or deprecating this prop or the entire old version of the component anyway, meaning it might be a good time to adopt the complete Ariakit API anyway. The point about speeding up the reakit migration makes sense to me though, so I'm happy to go with that approach. However, to me that only makes sense if we deprecate this component and do a fully new V2 with the new Ariakit API. Is that what you folks had in mind? |
|
I'm not sure that deprecating the current usage is compatible with the need for removing Given the similar conversation that we had around
Optionally, we could try to see if we can detect when Something like:const DisclosureContent = ( {
// Depretacted reakit props
baseId,
visible,
animated,
animating,
stopAnimation,
// New ariakit props
...props
} ) {
let fallbackReakitStore;
if (
// any of the deprecated props are defined
) {
// Add warning about legacy reakit props being deprecated, and that the component
// may not work as intended. Consumers should migrated to the ariakit APIs.
fallbackReakitStore = Ariakit.useDisclosureStore( {
animated,
open: visible,
} );
}
return <Ariakit.DisclosureContent {...props} store={props.store ?? fallbackReakitStore} />
}@diegohaz what do you think about this approach? |
|
For this PR, I'd concentrate solely on implementing the API that the old version already supported, maintaining the The decision to rename the prop to |
tyxla
reviewed
Nov 6, 2023
There was a problem hiding this comment.
Thanks for clarifying @ciampo and @diegohaz!
I'm happy with your plan, so marking my comments as resolved.
I've only left open a few comments regarding documentation, but those aren't blockers, so I'm retracting my blocking review.
Happy to leave the final review and approval to @ciampo and/or @diegohaz.
tyxla
dismissed
their stale review
My concerns were addressed in #55639 (comment)
flootr
force-pushed
the
feat/disclosure-content-ariakit
branch
2 times, most recently
from
a5bef21
to
5939d4a
Compare
|
Flaky tests detected in 5939d4a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6781432800
|
ciampo
approved these changes
Nov 7, 2023
| * Accessible Disclosure component that controls visibility of a section of | ||
| * content. It follows the WAI-ARIA Disclosure Pattern. | ||
| * | ||
| * @see https://ariakit.org/reference/disclosure-content |
There was a problem hiding this comment.
In the light of the approach chosen for this PR, it's probably better not to link to ariakit docs, since DisclosureContent wouldn't currently have the same public APIs as the ariakit component.
flootr
force-pushed
the
feat/disclosure-content-ariakit
branch
from
5939d4a
to
f59bcf3
Compare
|
Nice! As a follow-up, we can now look into introducing the full suite of |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
DisclosureContentfromreakittoariakitWhy?
Part of #53278
As stated in the linked issue Reakit does not explicitly support React 18 (causing peer dependency warnings) and Reakit has already been succeeded by Ariakit, so it will not receive any more updates.
How?
Switch out the Reakit dependency for Ariakit. We also have to create a small wrapper to maintain the existing API of the component.
Testing Instructions
This component doesn't come with a story and is used in only one place:
npm run devandnpm run wp-env startlocalhost:8888and activate a theme which supports widgetsAppearance>Widgetsand verify toggling widget areas still works as expected