Skip to content
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

Merged
merged 5 commits into from
Feb 9, 2023
Merged

Fix/autofocusing #1408

merged 5 commits into from
Feb 9, 2023

Conversation

Jamchello
Copy link
Contributor

No description provided.

@Jamchello Jamchello marked this pull request as ready for review February 7, 2023 12:11
@Jamchello Jamchello requested a review from mholtzman February 8, 2023 15:10
app/dash/Accounts/Add/Components/index.js Outdated Show resolved Hide resolved
resources/Hooks/useAutofocus.js Outdated Show resolved Hide resolved
resources/Hooks/useAutofocus.js Outdated Show resolved Hide resolved
resources/Hooks/useAutofocus.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@mholtzman mholtzman left a 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

resources/Components/Password/index.js Outdated Show resolved Hide resolved
resources/Components/Password/index.js Outdated Show resolved Hide resolved
const useFocusableRef = (focus) => {
const ref = useRef(null)

useEffect(() => {
Copy link
Collaborator

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

Copy link
Contributor Author

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.

Copy link
Collaborator

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!

@Jamchello Jamchello requested a review from mholtzman February 9, 2023 12:08
Copy link
Collaborator

@mholtzman mholtzman left a 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(() => {
Copy link
Collaborator

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!

@Jamchello Jamchello merged commit 52d25f1 into develop Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants