Skip to content

Conversation

asenmitrev
Copy link

  • What does this PR do?
    Enables imperative focus on the otp input for the first empty input.

  • Any background context you want to provide?
    Imperative focus is needed often when focusing inputs from outside the component. It is one of the most common uses of imperative programming in React, so it makes sense to enable it.

@prateek3255
Copy link
Contributor

Hey @asenmitrev Sorry for the delay in reviewing here, can you resolve the conflicts here I would give it a run again and then will merge if it looks good

src/index.tsx Outdated
inputRef?.focus();
},
}),
[value, activeInput]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have we included both value and activeInput if we're only using value here in the method?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mistake, will fix

src/index.tsx Outdated
ref,
() => ({
focus() {
const inputRef = inputRefs.current[value.length] ?? inputRefs.current[0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also why are we relying on the length of the value, any reason for not doing it like this:

Suggested change
const inputRef = inputRefs.current[value.length] ?? inputRefs.current[0];
const inputRef = inputRefs.current[activeInput]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried using activeInput but the behaviour was not what seems like the best UX. Sometimes, the active input is not the first unfilled input, but a filled input before. So you would overwrite the last value of the otp when clicking focus. Using the value length allows to always focus the first unfilled input.

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.

3 participants