fix: radio, checkbox, switch touch and selection behavior #4311
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 233a0b9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Rate limit exceeded@jrgarciadev has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces patches for several components of the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
.changeset/violet-tools-refuse.md (1)
8-8: Fix typo and enhance changeset messageThe word "intreactive" contains a typo. Additionally, the changeset message could be more descriptive about the specific changes made.
Consider updating to:
-Fix #4252 #4260 intreactive elements were not working properly +Fix #4252 #4260 - Improve touch and selection behavior by removing VisuallyHidden wrapper from radio, checkbox, and switch componentspackages/core/theme/src/utils/classes.ts (2)
71-104: Well-structured hidden input implementation with room for improvementThe implementation is thorough and addresses key aspects of hidden input behavior. Some suggestions:
Consider these enhancements:
- Add CSS custom property for vertical hit area (--cursor-hit-y)
- Consider using
inset-0instead of individual positioning properties- Add touch-action properties for better mobile interaction
export const hiddenInputClasses = [ // Variables "[--cursor-hit-x:8px]", + "[--cursor-hit-y:8px]", // ... other classes ... // Positioning & Hit area - "absolute", - "top-0", - "start-[calc(var(--cursor-hit-x)*-1)]", - "w-[calc(100%+var(--cursor-hit-x)*2)]", - "h-full", + "absolute", + "inset-0", + "m-[calc(var(--cursor-hit-y)*-1)_calc(var(--cursor-hit-x)*-1)]", + "touch-action-manipulation", // ... remaining classes ... ];
96-97: Consider adjusting opacity for better touch feedbackThe current opacity (0.0001) might be too low for browsers to register touch feedback on some devices.
- "opacity-[0.0001]", + "opacity-[0.001]",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
.changeset/violet-tools-refuse.md(1 hunks)packages/components/checkbox/src/checkbox.tsx(1 hunks)packages/components/checkbox/src/use-checkbox.ts(1 hunks)packages/components/radio/src/radio.tsx(1 hunks)packages/components/radio/src/use-radio.ts(1 hunks)packages/components/switch/src/switch.tsx(1 hunks)packages/components/switch/src/use-switch.ts(2 hunks)packages/core/theme/src/components/checkbox.ts(2 hunks)packages/core/theme/src/components/radio.ts(2 hunks)packages/core/theme/src/components/toggle.ts(2 hunks)packages/core/theme/src/utils/classes.ts(1 hunks)packages/core/theme/src/utils/index.ts(1 hunks)
🔇 Additional comments (14)
packages/core/theme/src/utils/index.ts (1)
10-10: LGTM! Export addition aligns with PR objectives
The addition of hiddenInputClasses export is consistent with the existing pattern and supports the new hidden input styling approach.
packages/components/radio/src/radio.tsx (1)
23-23: Verify accessibility impact of removing VisuallyHidden wrapper
The direct input rendering simplifies the component structure and should improve touch/selection behavior. However, we should verify that this change maintains proper accessibility.
Let's verify the accessibility-related class names are properly applied:
✅ Verification successful
Let me gather more information about the accessibility implementation by checking the input props and related components.
Let me check one more aspect to verify how the input's accessibility is handled in the radio component.
Based on the gathered information, I can now provide the final response:
Removing VisuallyHidden wrapper is safe for accessibility
The removal of the VisuallyHidden wrapper doesn't impact accessibility because:
- The component properly implements all necessary ARIA attributes through getInputProps, including aria-label, aria-labelledby, aria-describedby, and aria-required
- The radio component has comprehensive test coverage verifying these accessibility attributes
- The input element's accessibility is maintained through proper ARIA attributes rather than relying on visual hiding techniques
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if hiddenInputClasses are properly defined and contain necessary a11y properties
ast-grep --pattern 'export const hiddenInputClasses = {
$$$
}'
# Check for any existing aria-related attributes in the radio component
rg -A 2 'aria-|role=' packages/components/radio/
Length of output: 9108
Script:
#!/bin/bash
# Check getInputProps implementation
ast-grep --pattern 'const getInputProps = $$$'
# Check if there are any specific styles or classes related to input visibility
rg -A 3 'input.*class|visually-hidden|srOnly' packages/components/radio/
# Check if there are any specific styles applied to the input element
ast-grep --pattern 'const input = {
$$$
}'
Length of output: 2134
Script:
#!/bin/bash
# Check the implementation of useRadio hook for input styling and accessibility
ast-grep --pattern 'const useRadio = ($$$) => {
$$$
}'
# Check if there are any specific CSS classes or styles for the input
rg -A 5 'const.*styles.*=.*{' packages/components/radio/src/
Length of output: 223
packages/components/checkbox/src/checkbox.tsx (1)
28-28: Verify accessibility after VisuallyHidden removal
While removing the VisuallyHidden wrapper might improve touch/selection behavior, we should verify that the input remains accessible to screen readers.
Consider adding automated accessibility tests to prevent regressions in the future.
✅ Verification successful
Let me gather more information about the checkbox component implementation to verify the accessibility aspects.
Accessibility is properly maintained after VisuallyHidden removal
The removal of VisuallyHidden wrapper is safe because:
- The component uses React Aria's useCheckbox hook which provides proper ARIA attributes (
aria-label,aria-labelledby, etc.) - The input element receives all necessary accessibility props through
getInputProps() - The label is properly associated with the input through a generated labelId
- All interactive states (focus, hover, disabled) are correctly managed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any aria-* attributes or role attributes that might have been affected
rg -A 3 'aria-|role=' packages/components/checkbox/
Length of output: 15255
Script:
#!/bin/bash
# Check the implementation of useCheckbox hook to understand how input props are handled
ast-grep --pattern 'const useCheckbox = $_$ => {
$$$
}'
# Check if there are any test cases specifically for accessibility
rg -l "accessibility|screen reader|a11y" packages/components/checkbox/__tests__/
Length of output: 142
Script:
#!/bin/bash
# Let's check the actual implementation of the checkbox component and its hook
cat packages/components/checkbox/src/use-checkbox.ts
cat packages/components/checkbox/src/checkbox.tsx
# Also check if there are any specific roles or ARIA attributes being used
rg -A 3 'getInputProps|inputProps' packages/components/checkbox/
Length of output: 14485
packages/components/switch/src/switch.tsx (1)
37-37: Verify touch target size and positioning
The direct input rendering should improve touch interaction. Ensure the input's touch target:
- Covers the entire switch area
- Doesn't interfere with adjacent elements
- Maintains proper hit testing with the thumb component
packages/core/theme/src/components/toggle.ts (2)
4-4: LGTM: Import addition aligns with component enhancement
The addition of hiddenInputClasses import is consistent with the implementation of accessible form controls.
57-57: LGTM: Hidden input slot enhances accessibility
The addition of the hiddenInput slot with hiddenInputClasses improves the component's accessibility by providing proper styling for the hidden input element.
packages/core/theme/src/components/radio.ts (2)
4-4: LGTM: Import addition is consistent
The addition of hiddenInputClasses import aligns with the component enhancement pattern.
47-47: LGTM: Hidden input slot improves accessibility
The addition of the hiddenInput slot enhances the radio component's accessibility by providing proper styling for the hidden input element.
packages/core/theme/src/components/checkbox.ts (2)
4-4: LGTM: Import addition follows pattern
The addition of hiddenInputClasses import is consistent with the component enhancement pattern.
54-55: LGTM: Enhanced accessibility and interaction behavior
The changes improve the component by:
- Adding the
hiddenInputslot for proper accessibility - Adding
pointer-events-noneto the icon to prevent unwanted interactions
packages/components/radio/src/use-radio.ts (1)
205-205: LGTM! Verify touch behavior on mobile devices.
The addition of hiddenInput class styling capability is a good improvement. This change aligns with the PR's objective of fixing touch and selection behavior.
Let's verify if there are any existing touch-related issues:
✅ Verification successful
LGTM! The hidden input class addition is safe and well-tested.
The change to add className for hiddenInput is safe as:
- The component has comprehensive test coverage for click interactions and accessibility
- The radio component properly handles user interactions through both mouse and keyboard
- The implementation follows proper ARIA patterns with role="radio" and required accessibility attributes
- Touch behavior is handled through the default browser behavior for radio inputs, which is enhanced by the proper styling of the hidden input
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for touch-related issues or discussions
gh issue list --search "touch radio in:title,body" --limit 5
Length of output: 62
Script:
#!/bin/bash
# Search for touch-related code and event handlers in radio component
rg -i "touch|ontouchstart|ontouchend" packages/components/radio/
# Look for any existing test files related to touch events
fd -e test.tsx -e test.ts -e spec.tsx -e spec.ts . packages/components/radio/ --exec rg -l "touch"
# Check for any touch-related accessibility attributes or ARIA properties
rg -i "aria.*touch|role.*button" packages/components/radio/
Length of output: 323
Script:
#!/bin/bash
# Check the implementation of the radio component for any click or pointer event handlers
ast-grep --pattern 'onClick = $_' packages/components/radio/
rg -i "onpress|onclick|pointer" packages/components/radio/
# Look at the radio component's test coverage
cat packages/components/radio/__tests__/radio.test.tsx
Length of output: 13265
packages/components/switch/src/use-switch.ts (2)
165-165: LGTM! Improves code readability.
The added empty line improves code readability by separating logical blocks.
213-213: LGTM! Consistent with radio component implementation.
The addition of hiddenInput class styling capability maintains consistency across components and supports the touch behavior improvements.
Let's verify the consistency of hidden input implementation across components:
✅ Verification successful
Hidden input implementation is consistent across radio, checkbox, and switch components
The verification confirms that the hiddenInput class styling implementation through slots.hiddenInput({class: classNames?.hiddenInput}) is consistently used across all relevant form components:
- packages/components/radio/src/use-radio.ts
- packages/components/checkbox/src/use-checkbox.ts
- packages/components/switch/src/use-switch.ts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for hiddenInput usage across components
ast-grep --pattern 'slots.hiddenInput({$$$})'
Length of output: 408
packages/components/checkbox/src/use-checkbox.ts (1)
313-316: LGTM! Proper implementation of React hooks dependencies.
The changes are well-implemented:
- Addition of
hiddenInputclass styling maintains consistency across components - Including
classNames?.hiddenInputin the dependency array ensures proper memoization
Let's verify the consistency of dependency arrays across components:
✅ Verification successful
Dependency array update is correctly implemented
The changes in the checkbox component's dependency array are consistent with React's best practices. After examining the codebase:
- The
classNames?.hiddenInputdependency is correctly added to thegetInputPropsdependency array since it's used within the function. - Similar patterns are found in the radio and switch components where style-related props are included in their respective dependency arrays.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for useCallback hooks with classNames dependencies
ast-grep --pattern 'useCallback($$$, [$$$classNames$$$])'
Length of output: 59
Script:
#!/bin/bash
# Let's try a different approach to find dependency arrays with classNames
rg "useCallback.*classNames" -A 2
# Also search for similar patterns in the checkbox component
rg "useCallback.*\[.*\]" packages/components/checkbox/src/use-checkbox.ts -A 1
Length of output: 116
Script:
#!/bin/bash
# Let's examine the checkbox component's hooks and dependencies
cat packages/components/checkbox/src/use-checkbox.ts
# Also check similar patterns in radio and switch components for consistency
fd "use-radio.ts|use-switch.ts" --exec cat {}
Length of output: 25494
📝 Description
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No): No
📝 Additional Information
Summary by CodeRabbit
New Features
hiddenInputslot for improved accessibility and interaction in checkbox, radio, and toggle components.Bug Fixes
Documentation