fix(numeric): update demo problem and numeric event #34
Conversation
WalkthroughTwo changes: the demo ActionSheet now controls visibility explicitly in cancel/confirm handlers instead of relying on update events; the mobile numeric input adds touchstart handlers on increase/decrease controls to trigger existing actions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Demo as custom-content.vue
participant Sheet as TinyActionSheet
User->>Demo: Tap "Open"
Demo->>Demo: boxVisibility = true
Demo->>Sheet: Render with visible=true
Sheet-->>User: Sheet visible
alt User taps Cancel/Confirm
User->>Demo: cancelFn()/confirmFn()
Demo->>Demo: boxVisibility = false
Demo->>Sheet: visible=false
Sheet-->>User: Sheet closes
end
note over Demo,Sheet: No @update:visible binding<br/>Visibility managed solely by local state
sequenceDiagram
autonumber
actor User
participant Numeric as Numeric Component
par Decrease
User->>Numeric: touchstart on "−"
Numeric->>Numeric: decrease()
Numeric-->>User: Value decremented
and Increase
User->>Numeric: touchstart on "+"
Numeric->>Numeric: increase()
Numeric-->>User: Value incremented
end
note over Numeric: Touch events mirror existing click/keyboard behavior
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (3)
packages/demos/app/actionsheet/custom-content.vue (2)
23-25: Remove unused ref.
activeNameis declared but unused; trim it to keep the demo lean.-const activeName = ref('') const boxVisibility = ref(false)
31-34: Type the confirm payload for clarity.Add a type to
msgso TS users see the expected shape when copying this demo.-function confirmFn(msg) { +function confirmFn(msg: unknown) { console.log('确定:', msg) }If you know the ActionSheet’s
confirmpayload type (e.g., string | number), use that instead ofunknown.packages/mobile/components/numeric/src/mobile.vue (1)
25-35: Improve accessibility: make controls focusable and reflect disabled state.Spans with
role="button"aren’t tabbable by default; addtabindexandaria-disabledso keyboard and AT users can operate them.<span class="tiny-mobile-numeric__decrease" role="button" + :tabindex="state.minDisabled ? -1 : 0" + :aria-disabled="String(state.minDisabled)" v-if="controls" v-repeat-click="decrease" :class="{ 'is-disabled': state.minDisabled }" @keydown.enter="decrease" @@ <span class="tiny-mobile-numeric__increase" role="button" + :tabindex="state.maxDisabled ? -1 : 0" + :aria-disabled="String(state.maxDisabled)" v-if="controls" v-repeat-click="increase" :class="{ 'is-disabled': state.maxDisabled }" @keydown.enter="increase"Also applies to: 36-46
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/demos/app/actionsheet/custom-content.vue(1 hunks)packages/mobile/components/numeric/src/mobile.vue(2 hunks)
🔇 Additional comments (1)
packages/demos/app/actionsheet/custom-content.vue (1)
4-8: I’ve added a script to confirm the current binding incustom-content.vueand compare it againstellipsis.vuewhich uses two-way visibility sync. Let me know the results so we can finalize the review recommendation.
| @touchstart.stop.prevent="decrease" | ||
| > |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Touch handler can double-fire and may bypass disabled logic; unify input path and guard.
- Risk of double invoke on some devices if
v-repeat-clickalso listens to touch/pointer. - Directly calling
decrease/increasehere can bypass any disabled checks the directive might enforce.
Option 1 — Keep touchstart but guard disabled:
- @touchstart.stop.prevent="decrease"
+ @touchstart.stop.prevent="state.minDisabled ? void 0 : decrease()"
@@
- @touchstart.stop.prevent="increase"
+ @touchstart.stop.prevent="state.maxDisabled ? void 0 : increase()"Option 2 — Prefer pointer events (covers mouse/stylus/touch) and guard:
- @touchstart.stop.prevent="decrease"
+ @pointerdown.stop.prevent="state.minDisabled ? void 0 : decrease()"
@@
- @touchstart.stop.prevent="increase"
+ @pointerdown.stop.prevent="state.maxDisabled ? void 0 : increase()"Follow-up:
- If
v-repeat-clickcan be updated to use pointer events and handle disabled internally, consider removing these inline handlers to avoid divergent paths. I can draft that change if helpful.
Also applies to: 43-44
🏁 Script executed:
#!/bin/bash
# Display context around the handlers in mobile.vue (lines 1-100)
sed -n '1,100p' packages/mobile/components/numeric/src/mobile.vue
# Find all occurrences of the custom directive and its usage
rg -n "repeat-click" -C3Length of output: 5132
🏁 Script executed:
#!/bin/bash
# Locate the repeat-click directive implementation
find packages -type f | grep -E "repeat-click\\.(js|ts)$"Length of output: 101
🏁 Script executed:
#!/bin/bash
# Show the beginning of the repeat-click directive implementation
sed -n '1,200p' packages/mobile/utils/deps/repeat-click.ts
# Inspect which events are registered by the directive
rg -n "addEventListener" -C2 packages/mobile/utils/deps/repeat-click.ts
# Check for touch/pointer handling in the directive
rg -n "touch" -C2 packages/mobile/utils/deps/repeat-click.ts
rg -n "pointer" -C2 packages/mobile/utils/deps/repeat-click.ts
# Look for disabled logic within the directive
rg -n "disabled" -C2 packages/mobile/utils/deps/repeat-click.tsLength of output: 1436
Unify pointer & touch event handling and enforce disabled guard in numeric controls
Verified that the v-repeat-click directive in packages/mobile/utils/deps/repeat-click.ts only listens for mouse (mousedown/mouseup) and does not check for a disabled state, while the inline @touchstart handlers in the Vue template invoke decrease/increase unconditionally. This can lead to:
- Double-fire on touch devices that emit a synthetic
mousedownaftertouchstart. - Bypassing disabled logic, since neither the directive nor the inline handlers respect
state.minDisabled/state.maxDisabled.
Recommendations:
-
Refactor
v-repeat-clickto use pointer events and honor disabled state
Updatepackages/mobile/utils/deps/repeat-click.tsto registerpointerdown/pointerupand short-circuit when the control is disabled:// packages/mobile/utils/deps/repeat-click.ts - on(el, 'mousedown', (e) => { + on(el, 'pointerdown', (e) => { if (e.button !== 0) { return } + if (el.classList.contains('is-disabled')) { + return + } startTime = Date.now() once(document, 'mouseup', clear) clearInterval(interval) interval = setInterval(handler, LONG_PRESS_INTERVAL) })
Once the directive handles both mouse and touch (via pointer events) and respects the disabled state, you can remove the inline touch handlers below.
-
Quick inline-guard fix (if directive refactor isn’t immediate)
Change the touch handlers inpackages/mobile/components/numeric/src/mobile.vueto usepointerdownand skip when disabled:--- a/packages/mobile/components/numeric/src/mobile.vue +++ b/packages/mobile/components/numeric/src/mobile.vue <span class="tiny-mobile-numeric__decrease" role="button" v-if="controls" v-repeat-click="decrease" :class="{ 'is-disabled': state.minDisabled }" - @touchstart.stop.prevent="decrease" + @pointerdown.stop.prevent="state.minDisabled ? void 0 : decrease()" > … <span class="tiny-mobile-numeric__increase" role="button" v-if="controls" v-repeat-click="increase" :class="{ 'is-disabled': state.maxDisabled }" - @touchstart.stop.prevent="increase" + @pointerdown.stop.prevent="state.maxDisabled ? void 0 : increase()" >
This ensures both mouse and touch go through the same guard and prevents unintentional calls when disabled.
Affected locations:
packages/mobile/components/numeric/src/mobile.vue— lines ~32–33 and ~43–44packages/mobile/utils/deps/repeat-click.ts
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
packages/mobile/components/numeric/src/mobile.vue around lines 32–33 (and
similarly ~43–44): the inline @touchstart handlers fire unconditionally and can
double-trigger with mouse events and bypass disabled guards; replace the inline
touch handlers with pointerdown (or remove them after the directive update) and
short-circuit when appropriate by checking state.minDisabled/state.maxDisabled
before calling decrease/increase so disabled controls are honored; additionally
update packages/mobile/utils/deps/repeat-click.ts to listen to
pointerdown/pointerup (instead of mousedown/mouseup), consult the element's
disabled state (or a provided binding) and return early when disabled so the
directive and inline handlers share the same pointer-based behavior and disabled
guard.
James-9696
changed the title
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Bug Fixes