Toolbar: Use Ariakit instead of Reakit #51623
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
diegohaz
added
[Type] Enhancement
|
Size Change: +12.4 kB (+1%) Total Size: 1.41 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 0ce0baa. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5323819155
|
|
@diegohaz Please ping me when this is ready for accessibility testing. I doubt there will be any regressions but just to be sure. 👍 Thanks. |
|
Thank you, @alexstine! It's ready for review now. |
Closed
This increase in size should be mitigated when Reakit is fully replaced. I'll work on migrating the other parts after this PR is merged. |
|
Thank you so much for the reviews. I'll wait for @ciampo or @mirka's approval before merging this. While I understand the team is currently experimenting with Radix UI, migrating the components that currently rely on Reakit to Ariakit, now that Ariakit has achieved stability, will leave the project in a much better state. |
ciampo
reviewed
Jun 20, 2023
There was a problem hiding this comment.
Thank you for working on this refactor, Diego ❤️
Code changes LGTM, will test changes in the editor soon.
While I understand the team is currently experimenting with Radix UI, migrating the components that currently rely on Reakit to Ariakit, now that Ariakit has achieved stability, will leave the project in a much better state.
We are indeed experimenting with using Radix UI with some of our components, although that doesn't mean that we won't keep using Reakit/Ariakit, especially for its Toolbar-related components.
Are you planning on migrating the remaining reakit instances in the repo to ariakit, so that we can completely remove the reakit dependency from the repo?
| RefAttributes, | ||
| } from 'react'; | ||
|
|
||
| export interface ToolbarItemProps |
There was a problem hiding this comment.
We've been using type for components props in the package, should we continue using type for consistency?
There was a problem hiding this comment.
I updated it to use type to keep it consistent, but I recommend changing the convention to use interface when extending other types.
Intersection types may silently introduce incompatible types that will only show an error at usage. Also, interfaces merge JSDoc better: TS playground with more details.
There was a problem hiding this comment.
Thank you. That is a great point and something we may want to look at improving (cc @mirka )
There was a problem hiding this comment.
Yes, although there is a certain simplicity to sticking with type everywhere, I'm open to the "use interface unless something requires a type" guideline, suggested in the official docs and also pretty much at Google.
No need to overhaul anything in bulk, but maybe we can start by preferring interface in our own patches and not suggesting changes when a contributor uses interface?
There was a problem hiding this comment.
No need to overhaul anything in bulk, but maybe we can start by preferring interface in our own patches and not suggesting changes when a contributor uses interface?
Sounds good.
The reason why I suggested type is because my personal preference is to fallback to prefer consistency when we don't have a clear guideline on the matter.
But I agree with your point! We can definitely agree to go for your approach about interface from now on.
There was a problem hiding this comment.
fallback to prefer consistency when we don't have a clear guideline on the matter
Me too. Let's try this out low key, and we can add it to the CONTRIBUTING guidelines when we feel a bit more confident about it 😄
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
Yes, that's the plan. |
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
* Toolbar: Use Ariakit instead of Reakit * Add workaround * Add changelog * Update ariakit dependency * Update changelog * Update ToolbarItem JSDocs * interface -> type
This was referenced Aug 3, 2023
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?
This PR updates the Toolbar component from the
@wordpress/componentspackage to use Ariakit instead of Reakit. Ariakit is the successor of Reakit.Why?
Ariakit is already superior to Reakit in all aspects. It is faster, offers more features, improvements, and has fewer bugs. In fact, dozens of bugs have been reported in the past year, and all of them could be resolved in userland before a fix was implemented in the library. See closed bugs and notice that all of them have the "has workaround" label.
Ariakit provides full support for the latest versions of React (17 and 18). It is still released as v0.x.x because we are still working on the website and preparing for an official release.
Breaking changes are released as v0.x.0, following semver. Additionally, most of these changes are preceded by soft deprecations (without warnings), followed by deprecation warnings before the actual breaking change.
How?
I updated the Toolbar component to use the new API (mostly
state→store). I've also updated theToolbarItemprop types.Testing Instructions
The Toolbar component is mainly used in the
Block toolstoolbar (the block one) and theDocument toolstoolbar (the top one) on the editor. There should be no change in how they work.Those components are covered by the following e2e tests:
navigable-toolbar.spec.jsshortcut-focus-toolbar.spec.jstoolbar-roving-tabindex.spec.jsAll tests should pass.
Testing Instructions for Keyboard
The
shortcut-focus-toolbar.spec.jsandtoolbar-roving-tabindex.spec.jstests cover most of the keyboard interactions on the toolbar.Make sure you can navigate to the block toolbar using either ShiftTab on the block or AltF10. Once in the toolbar, you should be able to navigate through the items using arrow keys and interact with buttons by pressing Enter and Space. Dropdowns should open on the ArrowDown key and close with Esc, returning the focus to the dropdown button in the toolbar.
Interactions should also remain unchanged when the
Top toolbarsetting is enabled (in theOptionsmenu located at the end of theEditor top barregion). Note: As a sighted keyboard user, I noticed the behavior here is quite confusing. The visual position of the elements in the top toolbar (that now has both the block tools and document tools) doesn't seem to match the tab order. But this behavior is also present on trunk, so I guess it's out of the scope of this PR.