-
-
Notifications
You must be signed in to change notification settings - Fork 375
feat(OtpInput): use input instead of keydown event #7520
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
Conversation
optinput,三平台输入修正
|
Thanks for your PR, @Tony-ST0754. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
Reviewer's GuideRefactors the OTP input JavaScript to rely on the input event for value updates and focus management, simplifies key handling for navigation keys on keydown, and removes the beforeinput/keyup-based processing to improve Android keydown support and numeric-only behavior. Sequence diagram for updated OTP input handling on AndroidsequenceDiagram
actor User
participant Browser
participant OtpInputJs as OtpInputJs
participant DotNet as DotNetInvoker
User->>Browser: Tap OTP input
Browser->>OtpInputJs: focus event on bb-otp-item
OtpInputJs->>Browser: select current input value
User->>Browser: Press numeric key
Browser->>OtpInputJs: keydown event
OtpInputJs-->>Browser: allow key (no navigation key handling)
Browser->>OtpInputJs: input event with value
OtpInputJs->>OtpInputJs: normalize value (keep single digit)
OtpInputJs->>OtpInputJs: validate /^
OtpInputJs->>DotNet: setValue(invoke, method)
alt not last input
OtpInputJs->>Browser: focus next bb-otp-item
else last input
OtpInputJs->>Browser: blur current input
end
User->>Browser: Press Backspace/ArrowLeft
Browser->>OtpInputJs: keydown event
OtpInputJs->>Browser: setPrevFocus(el, target)
User->>Browser: Press ArrowRight
Browser->>OtpInputJs: keydown event
OtpInputJs->>Browser: setNextFocus(el, target)
Flow diagram for updated OTP input and keydown logicflowchart TD
A[Input event on bb-otp-item] --> B[Check type attribute]
B --> C{Is type number?}
C -->|Yes| D[If value length > 1, slice to single char]
C -->|No| E[Leave value as is]
D --> F{Does value match single digit ^\d$?}
E --> F
F -->|Yes| G[Call setValue and invoke .NET method]
G --> H{Is this last input field?}
H -->|No| I[Focus next bb-otp-item]
H -->|Yes| J[Blur current input]
F -->|No| K[Clear value]
subgraph Keydown navigation handling
K1[Keydown event on bb-otp-item] --> K2{ctrlKey pressed?}
K2 -->|Yes| K3[Return without handling]
K2 -->|No| K4{Key in skipKeys Enter Tab Shift Control Alt?}
K4 -->|Yes| K5[Do nothing]
K4 -->|No| K6{Key Backspace or ArrowLeft?}
K6 -->|Yes| K7[setPrevFocus]
K6 -->|No| K8{Key ArrowRight?}
K8 -->|Yes| K9[setNextFocus]
K8 -->|No| K10[No additional handling]
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey - I've left some high level feedback:
- In the keydown handler, the
isNumbervariable is never used and can be removed or the logic that depends on it restored to keep the handler focused and avoid confusion. - The new
skipKeysarray no longer includesDelete, and the specificDeletehandling inkeyupwas removed; ifDeleteis still expected to clear or move focus, consider reintroducing explicit handling for it in the new flow. - The
if (skipKeys.indexOf(e.key) > -1) { }branch in the keydown handler is effectively a no-op; either add an explicitreturnor simplify the condition to make the intent clearer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the keydown handler, the `isNumber` variable is never used and can be removed or the logic that depends on it restored to keep the handler focused and avoid confusion.
- The new `skipKeys` array no longer includes `Delete`, and the specific `Delete` handling in `keyup` was removed; if `Delete` is still expected to clear or move focus, consider reintroducing explicit handling for it in the new flow.
- The `if (skipKeys.indexOf(e.key) > -1) { }` branch in the keydown handler is effectively a no-op; either add an explicit `return` or simplify the condition to make the intent clearer.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7520 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 748 748
Lines 32986 32986
Branches 4584 4585 +1
=========================================
Hits 32986 32986
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Link issues
fixes #7518
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Improve OTP input behavior and keyboard handling, especially for Android keydown events.
New Features:
Bug Fixes:
Enhancements: