-
Notifications
You must be signed in to change notification settings - Fork 1.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
Add getOwnerDocument and getOwnerWindow utils and fix usePress #5096
Add getOwnerDocument and getOwnerWindow utils and fix usePress #5096
Conversation
files: ['packages/@react-aria/interactions/**/*.ts', 'packages/@react-aria/interactions/**/*.tsx'], | ||
rules: { | ||
'no-restricted-globals': [ | ||
WARN, |
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.
Choosing to warn here instead of error and only applying to @react-aria/interactions for now, will only switch to error once we have rolled out all the fixes.
cb2a0ff
to
d8af29b
Compare
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.
couple quick things, thanks for turning on the lint rule and the test, those will be a big help
]); | ||
}); | ||
|
||
test('should handle click events and ignore virtual point events', async () => { |
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 am not sure this test is helping, if I revert your changes, this one still passes. This is surprising to me. Do you know why that would be?
also, the click event is still fired in the non-virtual case as well, see https://codesandbox.io/s/musing-aryabhata-nhlwyf?file=/src/App.js to see what events the browser fires
I do love adding a test for virtual cursors in addition to pointer. We'll also need one for keyboard I think. Or if it's left off intentionally, that'd be good to know.
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 believe it's because there's no listener for the click event but only an onClick handler function so that is always fired since we target the element.
I'll add a test for the keyboard event.
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.
@snowystinger Added the test for keyboard event, hoping this is correct?
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.
Sorry, I don't see the keyboard event test. Just remember, the tests you add will help us to not break your code in the future. So even if you've traced the events and have determined you only need to fireEvent with a couple of them, it's better to fire as many as is reasonable. We may change the internals of usePress in the future and depend on those events. We'll try our best to point out those cases where it's better to add an extra seemingly unneeded event.
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.
Apologies, added them, lmk if there are other events I should be adding!
2f37ed9
to
e49d852
Compare
GET_BUILD |
Build successful! 🎉 |
## API Changes
unknown top level export { type: 'identifier', name: 'Column' } @react-aria/utilsuseFormReset-
+getOwnerDocument {
+ el: Element | null
+ returnVal: undefined
+} getOwnerDocument-
+getOwnerWindow {
+ el: Element | null
+ returnVal: undefined
+} |
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 your patience and thanks for getting this started.
This PR adds the
getOwnerDocument
andgetOwnerWindow
util function in @react-aria/utils. It then uses it inusePress.ts
as a test and adds a corresponding test inusePress.test.js
.This is a follow-up to #5070, which involves incremental changes.
Related to #4634
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project:
Stripe - Connect Embedded Components