-
Notifications
You must be signed in to change notification settings - Fork 47.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
[react-ui] Move experimental event+a11y work to react-ui package #16794
Conversation
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'm okay with this rename.
Have we reached out to siddharthkp about the likelihood of getting the react-ui package though? Looks like he's actively publishing to it.
Details of bundled changes.Comparing: 9691eb2...ace6a73 react-ui
|
@bvaughn We might decide to move this to the React package in the future, but for now this is all more of a package of experimental work we're playing around with internally. I do quite like |
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
|
||
import React from 'react'; | ||
import {tabFocusableImpl} from './TabbableScope'; | ||
import {useKeyboard} from 'react-events/keyboard'; | ||
import {useKeyboard} from '../../events/keyboard'; |
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.
Oops this is the non-type import that could get inlined
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.
This is okay. I plan on doing a follow up where I add the a11y components to build and will revise it there.
|
||
import React from 'react'; | ||
import {TabbableScope} from './TabbableScope'; | ||
import {useKeyboard} from 'react-events/keyboard'; | ||
import {useKeyboard} from '../../events/keyboard'; |
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.
Here too
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.
This is okay. I plan on doing a follow up where I add the a11y components to build and will revise it there.
I think this PR may have broken DevTools tests. Can you run Hm... I think CI runs this though so maybe my local Yarn is just corrupted.
|
I had a similar issue when deleting the "react-events" directory locally. Git doesn't register directory removals as a change, so maybe we should update the script to check if the directory is empty? |
This PR renamed the
react-events
package toreact-ui
and adds a top-level directory for each category. Thereact-events
work goes into a top-levelevents
directory, whilst all the accessibility component experiments go intoaccessibility
. This also allows us to move relevant other parts of our experimental UI work into React Interactions, and other projects, into other top-level directories ofreact-ui
without having to make them fit into existing directory structures (which is getting awkward).