-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[FocusTrap] Convert focus trap to typescript #35005
[FocusTrap] Convert focus trap to typescript #35005
Conversation
|
Thanks for working on this. Could you please also convert the test file to TS? As for the test_static failing, changing the JSDoc of isEnabled in FocusTrap.types.ts to:
should help. |
@michaldudak , I just finished with the test file and all the checks are passing now. |
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 it took so long. It got buried among other notifications.
It looks good in general. I have just a couple of minor remarks.
Remove casting Co-authored-by: Michał Dudak <[email protected]> Signed-off-by: Trizotti <[email protected]>
Co-authored-by: Michał Dudak <[email protected]> Signed-off-by: Trizotti <[email protected]>
Co-authored-by: Michał Dudak <[email protected]> Signed-off-by: Trizotti <[email protected]>
No problem, @michaldudak ... Thanks for the review. I added the suggested changes. |
@@ -77,7 +85,7 @@ describe('<FocusTrap />', () => { | |||
}); | |||
|
|||
it('should warn if the root content is not focusable', () => { | |||
const UnfocusableDialog = React.forwardRef((_, ref) => <div ref={ref} />); | |||
const UnfocusableDialog = React.forwardRef<HTMLInputElement>((_, ref) => <div ref={ref} />); |
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.
Shouldn't it be HTMLDivElement
?
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.
You're right. I just fixed it.
Co-authored-by: Michał Dudak <[email protected]> Signed-off-by: Trizotti <[email protected]>
Co-authored-by: Michał Dudak <[email protected]> Signed-off-by: Trizotti <[email protected]>
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.
Looks good! Thanks a lot for your work!
closes #34722