feat(PinInput): add delimiter option#6392
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements a visual separator feature for the PinInput component, allowing developers to insert delimiters (such as a bullet point or custom character) after a specified input position for improved readability. The implementation adds an optional Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
src/runtime/components/PinInput.vue (1)
152-156: Consider exposing delimiter styling through the themeuislots.The hardcoded
<span class="text-dimmed">•</span>and wrapper<span>aren't themeable viaui.delimiterlike the rest of the component (e.g.,ui.base,ui.root). For consistency with other components and to let users override spacing/color without resorting to the slot, consider adding adelimiterslot entry to the theme and applyingui.delimiter({ class: uiProp?.delimiter })here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/PinInput.vue` around lines 152 - 156, The delimiter markup in PinInput.vue currently uses a hardcoded <span class="text-dimmed">•</span>; change this to use the theme `ui.delimiter` slot so users can style it: when rendering the delimiter (where `delimiterPositions?.includes((index as number) + 1)`), apply the theme helper like `ui.delimiter({ class: uiProp?.delimiter })` to the wrapper/element instead of the fixed "text-dimmed" class and keep the named slot `delimiter` as fallback; also add a corresponding `ui.delimiter` entry to the theme config so consumers can override spacing/color consistently with `ui.base`/`ui.root`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/content/docs/2.components/pin-input.md`:
- Line 173: The "Delimiter" heading is for a new feature and needs the Soon
badge; update the heading "### Delimiter" to include the badge token
:badge{label="Soon" class="align-text-top"} immediately after the heading text
so it reads like the other new-feature headings in this doc (ensure you modify
the heading line that contains "### Delimiter").
In `@src/runtime/components/PinInput.vue`:
- Around line 152-156: The delimiter check in PinInput.vue uses
delimiterPositions?.includes((index as number) + 1) which can match a position
equal to length and render a trailing delimiter; update the condition to also
ensure the computed position is less than the total length (e.g., only render
when (index + 1) < length and delimiterPositions includes that position). Modify
the span's v-if to combine the includes check with a guard against positions >=
length so delimiters are only inserted between inputs (referencing
delimiterPositions, index, and length in the PinInput component).
---
Nitpick comments:
In `@src/runtime/components/PinInput.vue`:
- Around line 152-156: The delimiter markup in PinInput.vue currently uses a
hardcoded <span class="text-dimmed">•</span>; change this to use the theme
`ui.delimiter` slot so users can style it: when rendering the delimiter (where
`delimiterPositions?.includes((index as number) + 1)`), apply the theme helper
like `ui.delimiter({ class: uiProp?.delimiter })` to the wrapper/element instead
of the fixed "text-dimmed" class and keep the named slot `delimiter` as
fallback; also add a corresponding `ui.delimiter` entry to the theme config so
consumers can override spacing/color consistently with `ui.base`/`ui.root`.
🪄 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: cc64eb2f-181d-46f7-9495-08a6538a6d46
⛔ Files ignored due to path filters (2)
test/components/__snapshots__/PinInput-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/PinInput.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
docs/content/docs/2.components/pin-input.mdplaygrounds/nuxt/app/pages/components/pin-input.vuesrc/runtime/components/PinInput.vue
commit: |
al1maher
changed the title
al1maher
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/runtime/components/PinInput.vue (1)
118-120: Trailing-delimiter guard looks good; drop redundantas numbercasts.The
(index as number) + 1 < looseToNumber(props.length)guard correctly prevents a dangling delimiter after the last input. Minor nit: sinceindexis already typed asnumberin the function signature, bothindex as numbercasts are no-ops and can be simplified.♻️ Proposed simplification
function shouldInsertDelimiter(index: number) { - return props.delimiterPositions?.includes((index as number) + 1) && (index as number) + 1 < looseToNumber(props.length) + return props.delimiterPositions?.includes(index + 1) && index + 1 < looseToNumber(props.length) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/PinInput.vue` around lines 118 - 120, The function shouldInsertDelimiter currently contains redundant "as number" casts on the already-typed parameter index; simplify by removing those casts and use index + 1 directly in both places while keeping the existing logic and guards (props.delimiterPositions?.includes(index + 1) && index + 1 < looseToNumber(props.length)); update references to props.delimiterPositions and props.length only (no other logic changes).docs/content/docs/2.components/pin-input.md (1)
211-215: Verify the blank line underdelimiter: |doesn't leak into the rendered slot.The literal block scalar preserves the empty line between
|and<span …>, which may render as a leading empty text node inside the delimiter slot (and potentially a stray line break in the demo). If the playground component-code loader trims it, this is fine — otherwise remove the blank line.♻️ Proposed tidy-up
slots: delimiter: | - <span class="text-primary font-bold">-</span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/content/docs/2.components/pin-input.md` around lines 211 - 215, The literal block under the YAML frontmatter for the delimiter slot contains an extra blank line after "delimiter: |" which can produce an unwanted leading empty text node; remove that blank line so the content starts immediately after "delimiter: |" (i.e., ensure the slot's literal block begins with "<span class=\"text-primary font-bold\">-</span>" on the next line) to prevent a stray empty node in the delimiter slot rendering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/components/PinInput.vue`:
- Around line 42-46: The component's paste/autofill isn't sanitizing delimiter
characters so pasted text like "123-456" can populate delimiter slots; add
normalization that strips any characters at delimiterPositions boundaries before
distributing into inputs: implement an onPaste handler on the root/input
elements that reads clipboard data, removes characters at indices corresponding
to delimiterPositions (or strips known delimiter chars), and then emits the
cleaned value via update:model-value (or calls the existing value-distribution
logic); also apply the same normalization to any external updates handled by the
update:model-value path so programmatic Autofill/paste-like updates are
normalized; reference PinInput.vue props/methods: delimiterPositions,
update:model-value event, and the input distribution logic to locate where to
integrate the sanitization.
---
Nitpick comments:
In `@docs/content/docs/2.components/pin-input.md`:
- Around line 211-215: The literal block under the YAML frontmatter for the
delimiter slot contains an extra blank line after "delimiter: |" which can
produce an unwanted leading empty text node; remove that blank line so the
content starts immediately after "delimiter: |" (i.e., ensure the slot's literal
block begins with "<span class=\"text-primary font-bold\">-</span>" on the next
line) to prevent a stray empty node in the delimiter slot rendering.
In `@src/runtime/components/PinInput.vue`:
- Around line 118-120: The function shouldInsertDelimiter currently contains
redundant "as number" casts on the already-typed parameter index; simplify by
removing those casts and use index + 1 directly in both places while keeping the
existing logic and guards (props.delimiterPositions?.includes(index + 1) &&
index + 1 < looseToNumber(props.length)); update references to
props.delimiterPositions and props.length only (no other logic changes).
🪄 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: a7f4c71d-731c-4320-99bf-0aa111fef563
📒 Files selected for processing (2)
docs/content/docs/2.components/pin-input.mdsrc/runtime/components/PinInput.vue
| /** | ||
| * Position(s) after which to insert a delimiter. | ||
| * @example [3, 6] // Insert delimiters after 3rd and 6th inputs | ||
| */ | ||
| delimiterPositions?: number[] |
There was a problem hiding this comment.
Consider paste/autofill sanitization to fully address issue #6389.
The linked issue calls out robust paste/autofill handling — e.g., pasting "123-456" or "123 456" into a PIN that displays delimiters at position 3 should strip the delimiter characters and fill ["1","2","3","4","5","6"]. The current implementation is display-only (delimiter is a sibling <span>, so it never enters the inputs), which means:
- With
type="number", reka-ui already filters non-numeric input on paste — fine. - With
type="text", a paste of"123-456"will distribute-into the 4th slot rather than being treated as a delimiter and skipped.
If that's intentionally out of scope for this PR, consider calling it out as a known limitation in the docs; otherwise, an @paste/update:model-value normalization step that strips characters appearing at delimiterPositions boundaries would close the gap.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/runtime/components/PinInput.vue` around lines 42 - 46, The component's
paste/autofill isn't sanitizing delimiter characters so pasted text like
"123-456" can populate delimiter slots; add normalization that strips any
characters at delimiterPositions boundaries before distributing into inputs:
implement an onPaste handler on the root/input elements that reads clipboard
data, removes characters at indices corresponding to delimiterPositions (or
strips known delimiter chars), and then emits the cleaned value via
update:model-value (or calls the existing value-distribution logic); also apply
the same normalization to any external updates handled by the update:model-value
path so programmatic Autofill/paste-like updates are normalized; reference
PinInput.vue props/methods: delimiterPositions, update:model-value event, and
the input distribution logic to locate where to integrate the sanitization.
benjamincanac
left a comment
benjamincanac
left a comment
There was a problem hiding this comment.
Thanks for the PR!
Can we call it separator rather than delimiter for consistency? Matches our Separator component, plus delimiter already means something different in InputTags/InputMenu.
I'm also not sold on the positions array. Feels like every real use case is just regular grouping anyway, so could we make it a single :separator="3" instead?
Also the separator styling should live in the theme (a separator slot) instead of inline classes!
al1maher
force-pushed
the
feat/allow-delimiter-for-pin-input
branch
from
d5d521a to
b800b71
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/content/docs/2.components/pin-input.md`:
- Around line 201-216: Docs and implementation disagree on the delimiter slot:
the docs show using a slot named "delimiter" while the component (PinInput.vue)
exposes <slot name="separator">; update one side so names match. Either change
the component slot name from "separator" to "delimiter" inside PinInput.vue
(replace the <slot name="separator"> usage) or change the docs example to
instruct users to use the "separator" slot; reference PinInput.vue and the docs
file docs/content/docs/2.components/pin-input.md when making the change so both
use the same slot name.
🪄 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: 747f55b1-8c7b-4e0f-b628-ae75e2237a46
📒 Files selected for processing (2)
docs/content/docs/2.components/pin-input.mdplaygrounds/nuxt/app/pages/components/pin-input.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- playgrounds/nuxt/app/pages/components/pin-input.vue
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/content/docs/2.components/pin-input.md`:
- Line 175: The docs sentence describing the separator prop contains two
misspellings of "separator" as "seperator"; update the text that reads "Use the
`separator` prop to insert a seperator between specific input fields. Pass a
number indicating after which position to insert a seperator." to use the
correct spelling "separator" in both places (refer to the `separator` prop
string in the Markdown).
🪄 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: e448e4c0-e76b-4d00-9f48-ff2d5f900631
📒 Files selected for processing (4)
docs/app/components/content/examples/pin-input/PinInputSeparatorExample.vuedocs/content/docs/2.components/pin-input.mdplaygrounds/nuxt/app/pages/components/pin-input.vuetest/components/PinInput.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- playgrounds/nuxt/app/pages/components/pin-input.vue
|
|
||
| ### Separator :badge{label="Soon" class="align-text-top"} | ||
|
|
||
| Use the `separator` prop to insert a seperator between specific input fields. Pass a number indicating after which position to insert a seperator. |
There was a problem hiding this comment.
Fix typo in separator docs text.
Line 175 misspells “separator” twice as “seperator”.
✏️ Proposed fix
-Use the `separator` prop to insert a seperator between specific input fields. Pass a number indicating after which position to insert a seperator.
+Use the `separator` prop to insert a separator between specific input fields. Pass a number indicating after which position to insert a separator.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Use the `separator` prop to insert a seperator between specific input fields. Pass a number indicating after which position to insert a seperator. | |
| Use the `separator` prop to insert a separator between specific input fields. Pass a number indicating after which position to insert a separator. |
🧰 Tools
🪛 LanguageTool
[grammar] ~175-~175: Ensure spelling is correct
Context: ...} Use the separator prop to insert a seperator between specific input fields. Pass a n...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~175-~175: Ensure spelling is correct
Context: ...cating after which position to insert a seperator. ::component-code --- ignore: - placeho...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@docs/content/docs/2.components/pin-input.md` at line 175, The docs sentence
describing the separator prop contains two misspellings of "separator" as
"seperator"; update the text that reads "Use the `separator` prop to insert a
seperator between specific input fields. Pass a number indicating after which
position to insert a seperator." to use the correct spelling "separator" in both
places (refer to the `separator` prop string in the Markdown).
ad7b719 to
77a01e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/runtime/components/PinInput.vue`:
- Around line 121-125: The shouldInsertSeparator function uses props.separator
directly in a modulo operation which can break for 0, negatives, decimals or
non-numeric values; fix by coercing separator to a number (e.g., const sep =
looseToNumber(props.separator)), validate it with Number.isInteger(sep) && sep >
0, and only perform the existing modulo check when that validation passes (use
sep in place of props.separator), keeping the other length check with
looseToNumber(props.length) as-is.
🪄 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: afc33463-6b85-41b6-92c0-ce433116c94e
⛔ Files ignored due to path filters (2)
test/components/__snapshots__/PinInput-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/PinInput.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
docs/app/components/content/examples/pin-input/PinInputSeparatorExample.vuedocs/content/docs/2.components/pin-input.mdplaygrounds/nuxt/app/pages/components/pin-input.vuesrc/runtime/components/PinInput.vuesrc/theme/pin-input.ts
✅ Files skipped from review due to trivial changes (2)
- src/theme/pin-input.ts
- playgrounds/nuxt/app/pages/components/pin-input.vue
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/app/components/content/examples/pin-input/PinInputSeparatorExample.vue
| function shouldInsertSeparator(index: number) { | ||
| return props.separator !== undefined | ||
| && (index + 1) % props.separator === 0 | ||
| && (index + 1) < looseToNumber(props.length) | ||
| } |
There was a problem hiding this comment.
Validate separator as a positive integer before using modulo.
Line 123 currently accepts 0, negatives, and decimals, which can lead to non-intuitive grouping behavior (% 0 yields NaN, fractional intervals are ambiguous). Restrict separators to positive integers in shouldInsertSeparator.
Suggested patch
function shouldInsertSeparator(index: number) {
- return props.separator !== undefined
- && (index + 1) % props.separator === 0
+ const separator = props.separator
+ return Number.isInteger(separator)
+ && separator > 0
+ && (index + 1) % separator === 0
&& (index + 1) < looseToNumber(props.length)
}🤖 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 `@src/runtime/components/PinInput.vue` around lines 121 - 125, The
shouldInsertSeparator function uses props.separator directly in a modulo
operation which can break for 0, negatives, decimals or non-numeric values; fix
by coercing separator to a number (e.g., const sep =
looseToNumber(props.separator)), validate it with Number.isInteger(sep) && sep >
0, and only perform the existing modulo check when that validation passes (use
sep in place of props.separator), keeping the other length check with
looseToNumber(props.length) as-is.
Did just that! I'm not too sold in the array thing neither and just did that thinking that maybe there's a rare use case for it somehow, but anyhow I believe you're right and it's better to make it just a simple separator |
2 participants
Allow splitting up input to improve readability or consistency - depending on how a brand prefers to present the code.
🔗 Linked issue
Resolves #6389
❓ Type of change
📚 Description
📝 Checklist