feat: add OTP field component#802
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis pull request introduces a complete OTPField component to the Raystack UI library. The implementation wraps Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/www/src/content/docs/components/otp-field/index.mdx (1)
25-25: 💤 Low valueConsider clarifying the
lengthrequirement explanation.The phrase "before all slots hydrate" is slightly confusing. The
lengthprop is required so the component can:
- Size the input correctly
- Validate the complete value
- Detect when all slots are filled
Consider rewording to: "
lengthis required so the field can manage state, validate input, and detect completion."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/www/src/content/docs/components/otp-field/index.mdx` at line 25, Update the explanatory sentence about the length prop in the OTP field docs: replace the confusing phrase that mentions "before all slots hydrate" with a clearer description that references the length prop's responsibilities (e.g., "`length` is required so the field can manage state, validate input, and detect completion"). Locate the text in apps/www/src/content/docs/components/otp-field/index.mdx where the `length` prop is explained and swap the sentence while keeping the surrounding import/assembly guidance intact and still mentioning sizing, validation, and completion detection for the `length` prop.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/www/src/components/playground/otp-field-examples.tsx`:
- Around line 7-13: renderSlots currently computes aria-label totals using
length + offset, which produces incorrect "of" counts for grouped/separated
examples; change the function signature to accept a separate totalLength (e.g.,
renderSlots(length: number, offset = 0, totalLength = length)) and use
totalLength in the aria-label for OTPField.Input (use `Character ${i + 1 +
offset} of ${totalLength}`). Update all calls to renderSlots (including the
separator example and the other instance around lines 58-66) to pass the full
field length as the third argument so screen readers see the correct total.
---
Nitpick comments:
In `@apps/www/src/content/docs/components/otp-field/index.mdx`:
- Line 25: Update the explanatory sentence about the length prop in the OTP
field docs: replace the confusing phrase that mentions "before all slots
hydrate" with a clearer description that references the length prop's
responsibilities (e.g., "`length` is required so the field can manage state,
validate input, and detect completion"). Locate the text in
apps/www/src/content/docs/components/otp-field/index.mdx where the `length` prop
is explained and swap the sentence while keeping the surrounding import/assembly
guidance intact and still mentioning sizing, validation, and completion
detection for the `length` prop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 90a47a3a-70de-49f4-81d4-bbf6a32a4950
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
apps/www/src/components/playground/index.tsapps/www/src/components/playground/otp-field-examples.tsxapps/www/src/content/docs/components/otp-field/demo.tsapps/www/src/content/docs/components/otp-field/index.mdxapps/www/src/content/docs/components/otp-field/props.tspackages/raystack/components/otp-field/__tests__/otp-field.test.tsxpackages/raystack/components/otp-field/index.tsxpackages/raystack/components/otp-field/otp-field.module.csspackages/raystack/components/otp-field/otp-field.tsxpackages/raystack/index.tsxpackages/raystack/package.json
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/raystack/components/slider/__tests__/slider.test.tsx`:
- Around line 140-146: The test "sets aria-valuetext" currently only checks
existence of the input but not the attribute; update the assertion for the
Slider render (the test using render(<Slider value={50} aria-valuetext='50
percent' />), container.querySelector('input[type="range"]') and the slider
variable) to assert that the input has aria-valuetext equal to '50 percent'
(e.g., use expect(slider).toHaveAttribute('aria-valuetext', '50 percent') or
equivalent).
- Around line 79-83: The tests for Slider are inconsistent: "handles single
value" uses synchronous assertions but "handles range values" uses waitFor
unnecessarily; since both are controlled props their attributes should be set on
initial render, so remove the async waitFor from the "handles range values" test
and use the same synchronous pattern as in "handles single value" (query the
input[type=\"range\"] and assert aria-valuenow, aria-valuemin and aria-valuemax
directly). Locate the tests by their descriptions ("handles single value" and
"handles range values") in slider.test.tsx and update the latter to match the
synchronous assertions used in the former.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d82d8cc1-6039-4595-8587-7c0804e306f2
📒 Files selected for processing (2)
packages/raystack/components/slider/__tests__/slider.test.tsxpackages/raystack/index.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/raystack/index.tsx
1 participant
Summary
OTPFieldcomponent built on Base UI's OTP field with composableOTPField.InputandOTPField.Separatorslotslength,validationType,mask,disabled,readOnly,autoSubmit