-
Notifications
You must be signed in to change notification settings - Fork 155
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
Fix/autofocusing #1408
Fix/autofocusing #1408
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.
just a couple more nitpicks
const useFocusableRef = (focus) => { | ||
const ref = useRef(null) | ||
|
||
useEffect(() => { |
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.
can probably just wrap this whole thing in an if (focus)
so we dont need to invoke the hook and can get rid of the other conditional logic
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.
Wrapping the useEffect in an if caused a React error, moving this if(focus)
inside the hook does the trick and looks cleaner howver!
I have also added the delay parameter back as I realised on the initial input we can focus much quicker as there is no animation.
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.
ahh interesting, didn't know that. looks good!
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. can you add a few of these focus type cases to the testing tickets?
const useFocusableRef = (focus) => { | ||
const ref = useRef(null) | ||
|
||
useEffect(() => { |
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.
ahh interesting, didn't know that. looks good!
No description provided.