-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Toolbar: Use Ariakit instead of Reakit #51623
Conversation
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. |
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. |
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.
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.
@diegohaz This is testing well. 👍
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. |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄
Yes, that's the plan. |
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 🚀
* Toolbar: Use Ariakit instead of Reakit * Add workaround * Add changelog * Update ariakit dependency * Update changelog * Update ToolbarItem JSDocs * interface -> type
What?
This PR updates the Toolbar component from the
@wordpress/components
package 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 theToolbarItem
prop types.Testing Instructions
The Toolbar component is mainly used in the
Block tools
toolbar (the block one) and theDocument tools
toolbar (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.js
shortcut-focus-toolbar.spec.js
toolbar-roving-tabindex.spec.js
All tests should pass.
Testing Instructions for Keyboard
The
shortcut-focus-toolbar.spec.js
andtoolbar-roving-tabindex.spec.js
tests 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 toolbar
setting is enabled (in theOptions
menu located at the end of theEditor top bar
region). 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.