fix: modify the issue of radio and checkbox components being unclickable - #37
Conversation
WalkthroughUpdates demo to include a checkbox group with reactive selection. Adjusts mobile radio component accessibility and keyboard event propagation. Introduces heatmap sizing variables and styling for checkbox and radio inputs. Bumps package versions for mobile and theme-mobile. Minor formatting changes in renderless logic without behavior changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/mobile/components/radio/src/mobile.vue (1)
31-43: A11Y regression: removing aria-hidden from the hidden input creates duplicate controls for SRThe label has role="radio" and handles focus/keyboard. Without aria-hidden, screen readers will also expose the native input (even with tabindex="-1"), leading to duplicate announcements. Restore aria-hidden on the input (or drop the custom role and rely on the native input).
Minimal fix:
<input ref="radio" class="tiny-mobile-radio__original" :value="label" type="radio" v-model="state.model" @focus="state.focus = true" @blur="state.focus = false" @change="handleChange" :name="name" :disabled="state.isDisabled" tabindex="-1" + aria-hidden="true" />Alternative (bigger change): remove role/aria-* from the label, set tabindex="0" on the input, and use the native control for accessibility.
🧹 Nitpick comments (4)
packages/theme-mobile/src/radio/index.less (1)
140-144: Clickable heatmap fix makes sense; avoid over‑constraining with inset + width/heightRaising z-index and adding explicit heatmap sizing should restore clickability. Minor: using inset: 0 together with explicit width/height over-constrains absolute positioning; prefer anchoring with top/left only to avoid inconsistent UA resolution.
Apply:
&__original { opacity: 0; outline: 0; position: absolute; - z-index: 1; - inset: 0; - width: var(--tvm-Radio-heatmap-width); - height: var(--tvm-Radio-heatmap-height); + z-index: 1; + top: 0; + left: 0; + width: var(--tvm-Radio-heatmap-width); + height: var(--tvm-Radio-heatmap-height); margin: 0; }packages/demos/app/radio/event.vue (2)
7-12: Demo addition is clear; avoid
for spacingPrefer CSS margins over
for layout spacing in demos.- <br/> <br/> + <!-- spacing handled by CSS -->And add (or reuse) a margin above the group:
.demo-checkbox-group { display: flex; justify-content: flex-start; flex-wrap: nowrap; width: 200px; + margin-top: 16px; }
25-27: Fix param shadowing in changeActionAvoid shadowing outer
value. Rename the parameter.-function changeAction(value) { // NISVUE3 FIXME: value重复定义，请手工修改 - text.value = value === '1' ? '男' : '女' +function changeAction(newValue) { // NISVUE3 FIXME resolved + text.value = newValue === '1' ? '男' : '女' }packages/theme-mobile/src/checkbox/index.less (1)
22-23: Adding width: 100% changes layout; verify group alignment and overflow.Setting the root checkbox container to width: 100% while it remains display: inline-block can force full-row occupancy and may make margin-right ineffective or cause unexpected horizontal spacing in groups. Please verify:
- Checkbox groups (inline demos) don’t regress into unintended stacking or horizontal scroll.
- Long labels still wrap as expected given root white-space: nowrap and label white-space: pre-wrap.
If the intent is a full-row tap target, consider switching the root to block and neutralizing the right margin for cleaner layout.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/demos/app/radio/event.vue(1 hunks)packages/mobile/components/radio/src/mobile.vue(2 hunks)packages/mobile/components/radio/src/renderless/index.ts(1 hunks)packages/mobile/package.json(1 hunks)packages/theme-mobile/package.json(1 hunks)packages/theme-mobile/src/checkbox/index.less(2 hunks)packages/theme-mobile/src/checkbox/vars.less(1 hunks)packages/theme-mobile/src/radio/index.less(1 hunks)packages/theme-mobile/src/radio/vars.less(1 hunks)
🔇 Additional comments (10)
packages/theme-mobile/package.json (1)
3-3: Approve version bump for theme-mobiletheme-mobile (1.0.0-alpha.11.5) now matches vue-mobile; ensure the changelog/tag for 1.0.0-alpha.11.5 exists and that the build pipeline publishes both packages together.
packages/mobile/package.json (1)
4-4: Confirm versions are in lockstep Both@opentiny/vue-mobileand@opentiny/vue-theme-mobileare at1.0.0-alpha.11.5; ensure you publish them atomically to avoid mismatched installs.packages/mobile/components/radio/src/renderless/index.ts (1)
106-107: No-op changeOnly a trailing newline added. No behavior impact.
packages/mobile/components/radio/src/mobile.vue (2)
22-24: Space key handling on the label is correctKeeping Space to toggle radios matches ARIA Authoring Practices. LGTM.
45-47: Allowing keydown to bubble from label text is goodRemoving keydown.stop on the label text ensures keyboard events reach the label handler. Nice.
packages/theme-mobile/src/checkbox/vars.less (1)
38-41: Heatmap variables added—good for consistency and themingConsistent with radio vars. No concerns.
packages/theme-mobile/src/radio/vars.less (1)
24-27: Heatmap size variables added—LGTMMatches the checkbox pattern; enables design tuning without code changes.
packages/demos/app/radio/event.vue (2)
18-18: Imports are correct and minimalUsing TinyCheckbox and TinyCheckboxGroup alongside TinyRadio reads well.
22-22: Good: v-model default for checkedInitializing to an array demonstrates group binding clearly.
packages/theme-mobile/src/checkbox/index.less (1)
85-95: Heatmap custom properties defined and used consistently
Checkbox and radio both declare--tvm-*-heatmap-width/-heightin theirvars.lessand apply them to the&__originaloverlays inindex.less.
| inset: 0; | ||
| width: var(--tvm-Checkbox-heatmap-width); | ||
| height: var(--tvm-Checkbox-heatmap-height); | ||
| opacity: 0; | ||
| z-index: 2; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Over-constrained absolute positioning and duplicate opacity; simplify the hit-area overlay.
Using inset: 0 together with explicit width/height can over-constrain absolutely positioned elements and may not yield the intended full-row “heatmap.” Also, opacity: 0 is duplicated (Line 86 and Line 92). Recommend relying solely on inset: 0 to cover the entire container and remove the duplicate property.
Apply this diff:
- inset: 0;
- width: var(--tvm-Checkbox-heatmap-width);
- height: var(--tvm-Checkbox-heatmap-height);
- opacity: 0;
- z-index: 2;
+ inset: 0;
+ z-index: 2;Result: the invisible input fully overlays the component, maximizing clickability across icon + label without layout surprises.
🏁 Script executed:
#!/bin/bash
# Locate the checkbox LESS file
FILE=$(find . -type f -path '*/packages/theme-mobile/src/checkbox/index.less')
# Print lines 1–120 for context
sed -n '1,120p' "$FILE"
# Show all opacity declarations with line numbers
rg -n 'opacity' "$FILE"
# Show inset, width, and height declarations
rg -nE 'inset:|width:|height:' "$FILE"Length of output: 3261
Simplify hit-area overlay in &__original
Remove redundant width, height and duplicate opacity: 0, and rely on inset: 0 to fully cover the container while preserving z-index: 2.
- inset: 0;
- width: var(--tvm-Checkbox-heatmap-width);
- height: var(--tvm-Checkbox-heatmap-height);
- opacity: 0;
- z-index: 2;
+ inset: 0;
+ z-index: 2;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/theme-mobile/src/checkbox/index.less around lines 89 to 94, the
&__original hit-area overlay contains redundant declarations (explicit width and
height plus a duplicated opacity: 0) even though inset: 0 already covers the
container; remove the width and height properties and the duplicate opacity
declaration, leaving only inset: 0 and z-index: 2 (and a single opacity: 0 if
needed) so the overlay is simplified and behavior is preserved.
2 participants
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
Style
Refactor
Chores