Skip to content

Fix/autofocusing#1408

Merged
Jamchello merged 5 commits intodevelopfrom
fix/autofocusing
Feb 9, 2023
Merged

Fix/autofocusing#1408
Jamchello merged 5 commits intodevelopfrom
fix/autofocusing

Conversation

@Jamchello
Copy link
Copy Markdown
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
Copy link
Copy Markdown
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

const useFocusableRef = (focus) => {
const ref = useRef(null)

useEffect(() => {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
wakamex pushed a commit to wakamex/framed that referenced this pull request Mar 2, 2026
* useConditionalAutofocus hook

* allow password submission immediately if no errors

* empty the mouth

* increase timeout so that the styling isnt messed up, renaming, apply useEffect

* tidy up, add back delay param
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