-
-
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
[base-ui][Input] Add OTP input demo #40539
Conversation
Netlify deploy previewBundle size report |
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.
This is great! Design-wise looks good :)
@sai6855 @michaldudak @zanivan, I believe it would be fantastic to introduce an additional base-ui hook specifically designed for handling OTP inputs. Chakra already offers a |
@marcpachecog that sounds like a good idea. Could you please open a feature request issue for it? 😊 |
@DiegoAndai Done! mui/base-ui#75 |
Great idea! I have a couple of remarks regarding functionality:
|
fixed in 23ee497
fixed in 51965ca
fixed in d67da7c
fixed in b8b382f |
|
||
const handleKeyDown = (event, currentIndex) => { | ||
switch (event.key) { | ||
case 'ArrowLeft': |
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.
Please handle ArrowUp the same way as ArrowLeft and ArrowDown the same way as ArrowRight
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.
Both ArrowUp
and ArrowLeft
does nothing (not even deselection of input) in mentioned benchmarks . so i reflected same behaviour in bb2f3a8
import { Input as BaseInput } from '@mui/base/Input'; | ||
import { Box, styled } from '@mui/system'; | ||
|
||
function OTP({ seperator, inputCount, otp, setOtp }) { |
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.
otp
and setOpt
are not really a standard way of providing a value. Could you change it to value
and onChange
?
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.
fixed here 7c6a232
selectInput(currentIndex); | ||
}; | ||
|
||
const handlePaste = (event, currentIndex) => { |
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.
This works great! I'm wondering if pasting in any of the digits shouldn't replace the whole value, though (so if your value is [9][9][9][9][9], have a cursor in the third input and have 12345 in the clipboard, the component should display [1][2][3][4][5] instead of [9][9][1][2][3]), but I'm not 100% sure about that.
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.
Both mentioned benchmarks displays [9][9][1][2][3] on pasting. so i reflected same behaviour.
setOtp={setOtp} | ||
inputCount={inputCount} | ||
/> | ||
</Box> |
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.
Could we display the entered code as plain text below the OTP component?
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.
fixed here 50c9913
|
||
export default function OTPInput() { | ||
const inputCount = 5; | ||
const [otp, setOtp] = React.useState<string[]>(new Array(inputCount).fill('')); |
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.
Having just a string here instead of an array would be more convenient. The value would usually be sent to server as a single string.
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.
done
}} | ||
> | ||
<OTP | ||
seperator={<span>-</span>} |
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.
Typo:
seperator={<span>-</span>} | |
separator={<span>-</span>} |
seperator={<span>-</span>} | ||
value={otp} | ||
onChange={setOtp} | ||
inputCount={inputCount} |
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.
length
could be more intiutive (+ shorter)
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.
since otp
is string now, couldn't use otp.length
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.
I meant the prop name
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.
Opps sorry.. fixed in this commit 7dee93b
ping @michaldudak |
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 OK to me!
Demo: https://deploy-preview-40539--material-ui.netlify.app/base-ui/react-input/#otp-input
looking at downloads of https://www.npmjs.com/package/react-otp-input and considering its applicability in various everyday scenarios, I believe this component warrants inclusion as the demo.
few Inspiration/ benchmarks:
https://github.com/devfolioco/react-otp-input
https://github.com/shubhanus/otp-input-react
I have followed (at least) the PR section of the contributing guide.