fix: fixing the non selectable issue #4306
Conversation
🦋 Changeset detectedLatest commit: 8dc580a The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces patches for the Changes
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
|
macci001
force-pushed
the
macci001/4252
branch
2 times, most recently
from
046cb2e to
8eb3445
Compare
macci001
force-pushed
the
macci001/4252
branch
from
8eb3445 to
c55efe9
Compare
macci001
force-pushed
the
macci001/4252
branch
from
c55efe9 to
56ad949
Compare
macci001
changed the title
macci001
force-pushed
the
macci001/4252
branch
from
d86febb to
8dc580a
Compare
![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
🧹 Outside diff range and nitpick comments (7)
packages/components/checkbox/src/use-checkbox.ts (1)
331-331: Consider adding screen reader focus indicationWhile the current implementation correctly handles touch interactions with absolute positioning and proper z-index, consider adding
focus:ringutility classes for better screen reader focus indication.- className: "absolute inset-0 opacity-0 z-50 cursor-pointer", + className: "absolute inset-0 opacity-0 z-50 cursor-pointer focus:ring-2 focus:ring-offset-2 focus:ring-primary",packages/components/radio/src/use-radio.ts (1)
142-143: Correct the reference in the comment fromuseCheckboxtouseRadio.The comment incorrectly mentions
useCheckbox; it should refer touseReactAriaRadioto accurately reflect the function being used.Apply this diff to fix the comment:
- // Handle press state for full label. Keyboard press state is returned by useCheckbox + // Handle press state for full label. Keyboard press state is returned by useReactAriaRadio.changeset/eighty-fireants-give.md (1)
7-7: Hyphenate "non touchable" to "non-touchable".Per standard spelling, "non-touchable" should be hyphenated.
Apply this diff to correct the spelling:
-Fixing the non touchable components issue(#4252) +Fixing the non-touchable components issue (#4252)🧰 Tools
🪛 LanguageTool
[grammar] ~7-~7: This expression is usually spelled with a hyphen.
Context: ...extui-org/radio": patch --- Fixing the non touchable components issue(#4252)(NON_ANTI_JJ)
apps/docs/content/components/radio-group/custom-impl.raw.jsx (1)
Line range hint
15-21: Optimize touch interaction areaThe custom radio implementation uses padding and gap for spacing, which affects the touch target area.
Consider adjusting the layout to ensure consistent touch targets:
-"max-w-[300px] cursor-pointer border-2 border-default rounded-lg gap-4 p-4", +"max-w-[300px] cursor-pointer border-2 border-default rounded-lg gap-4 p-4 min-h-[48px]",packages/components/switch/__tests__/switch.test.tsx (1)
188-202: Consider adding more edge cases to the focus testWhile the current test verifies basic focus behavior, consider adding tests for:
- Multiple rapid clicks
- Focus behavior with disabled state
- Focus behavior with readonly state
+ it("should not trigger focus on parent when disabled", async () => { + const onFocus = jest.fn(); + const wrapper = render( + <div tabIndex={-1} onFocus={onFocus}> + <Switch isDisabled data-testid="switch-test">Switch</Switch> + </div> + ); + const label = wrapper.getByTestId("switch-test"); + await user.click(label); + expect(onFocus).not.toHaveBeenCalled(); + });packages/components/checkbox/__tests__/checkbox.test.tsx (1)
93-107: Consider enhancing test robustnessWhile the test correctly verifies the focus behavior, consider these enhancements for better reliability:
- Add assertion to verify the checkbox state after click
- Add negative test case to ensure focus isn't triggered when parent isn't focusable
it("should trigger focus on focusable parent once after click", async () => { const onFocus = jest.fn(); + const onChange = jest.fn(); const wrapper = render( <div tabIndex={-1} onFocus={onFocus}> - <Checkbox data-testid="checkbox-test">Checkbox</Checkbox> + <Checkbox data-testid="checkbox-test" onChange={onChange}>Checkbox</Checkbox> </div>, ); const label = wrapper.getByTestId("checkbox-test"); + const input = wrapper.container.querySelector("input"); await user.click(label); expect(onFocus).toHaveBeenCalledTimes(1); + expect(onChange).toHaveBeenCalledTimes(1); + expect(input).toBeChecked(); }); +it("should not trigger focus on non-focusable parent after click", async () => { + const onFocus = jest.fn(); + + const wrapper = render( + <div onFocus={onFocus}> + <Checkbox data-testid="checkbox-test">Checkbox</Checkbox> + </div>, + ); + + const label = wrapper.getByTestId("checkbox-test"); + + await user.click(label); + + expect(onFocus).not.toHaveBeenCalled(); +});packages/components/radio/__tests__/radio.test.tsx (1)
145-163: Enhance test coverage for radio focus behaviorWhile the test verifies basic focus behavior, consider these enhancements for more comprehensive coverage:
- Verify radio selection state remains unchanged after focus
- Test focus behavior when switching between radio options
it("should trigger focus on focusable parent once after click", async () => { const onFocus = jest.fn(); + const onValueChange = jest.fn(); const wrapper = render( <div tabIndex={-1} onFocus={onFocus}> - <RadioGroup defaultValue="1" label="Options"> + <RadioGroup defaultValue="1" label="Options" onValueChange={onValueChange}> <Radio data-testid="radio-test-1" value="1"> Option 1 </Radio> + <Radio data-testid="radio-test-2" value="2"> + Option 2 + </Radio> </RadioGroup> </div>, ); const label = wrapper.getByTestId("radio-test-1"); + const radio1 = wrapper.container.querySelector('input[value="1"]'); + const radio2 = wrapper.container.querySelector('input[value="2"]'); await user.click(label); expect(onFocus).toHaveBeenCalledTimes(1); + expect(radio1).toBeChecked(); + expect(radio2).not.toBeChecked(); + + // Test focus behavior when switching options + await user.click(wrapper.getByTestId("radio-test-2")); + expect(onFocus).toHaveBeenCalledTimes(2); + expect(onValueChange).toHaveBeenCalledWith("2"); + expect(radio1).not.toBeChecked(); + expect(radio2).toBeChecked(); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (17)
.changeset/eighty-fireants-give.md(1 hunks)apps/docs/content/components/checkbox/custom-implementation.raw.jsx(2 hunks)apps/docs/content/components/radio-group/custom-impl.raw.jsx(2 hunks)apps/docs/content/components/radio-group/custom-impl.raw.tsx(2 hunks)apps/docs/content/components/switch/custom-impl.raw.jsx(2 hunks)apps/docs/content/components/switch/custom-impl.raw.tsx(2 hunks)packages/components/checkbox/__tests__/checkbox.test.tsx(1 hunks)packages/components/checkbox/src/checkbox.tsx(1 hunks)packages/components/checkbox/src/use-checkbox.ts(6 hunks)packages/components/radio/__tests__/radio.test.tsx(1 hunks)packages/components/radio/src/radio.tsx(1 hunks)packages/components/radio/src/use-radio.ts(5 hunks)packages/components/radio/stories/radio.stories.tsx(1 hunks)packages/components/switch/__tests__/switch.test.tsx(1 hunks)packages/components/switch/src/switch.tsx(1 hunks)packages/components/switch/src/use-switch.ts(4 hunks)packages/components/switch/stories/switch.stories.tsx(1 hunks)
🧰 Additional context used
🪛 LanguageTool
.changeset/eighty-fireants-give.md
[grammar] ~7-~7: This expression is usually spelled with a hyphen.
Context: ...extui-org/radio": patch --- Fixing the non touchable components issue(#4252)
(NON_ANTI_JJ)
🔇 Additional comments (14)
packages/components/checkbox/src/use-checkbox.ts (3)
215-230: Well-implemented press state management for touch interactions!
The separation of keyboard and mouse/touch press states is cleanly implemented, directly addressing the touch event issues mentioned in the PR objectives. The implementation correctly handles press state updates only for non-keyboard interactions.
232-232: Clean implementation of combined press state!
The press state combination elegantly handles both keyboard and mouse/touch interactions while respecting the disabled state, providing a single source of truth for the UI.
296-296: Verify touch event handling across different devices
The press event handlers are correctly merged with other props. However, let's verify the touch event behavior across different devices and browsers.
Also applies to: 312-312
✅ Verification successful
Let me gather more specific information about the touch event handling in the checkbox component.
Touch event handling implementation is properly integrated
The implementation shows correct touch event handling through React Aria's usePress hook:
- Press state is properly managed for non-keyboard interactions
- The
pressPropsare correctly merged with other props - Touch events are handled through the abstraction provided by React Aria's interactions
- Data attributes are properly set for pressed state
The implementation follows best practices for touch interactions and accessibility:
- Disabled state is respected for touch interactions
- Keyboard vs touch/pointer interactions are properly distinguished
- Press state is correctly synchronized with the component's visual feedback
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for touch event related tests and implementations
# Look for touch event related tests
echo "Searching for touch event related tests..."
rg -l "touch|press|pointer" --glob "**/*.{test,spec}.{ts,tsx,js,jsx}"
# Look for touch event implementations
echo "Searching for touch event implementations..."
ast-grep --pattern 'usePress|onTouchStart|onTouchEnd|onPointerDown|onPointerUp'
Length of output: 915
Script:
#!/bin/bash
# Search for touch/press event handling in checkbox component and its tests
echo "Searching checkbox component tests..."
rg -l "touch|press|pointer" packages/components/checkbox/__tests__/
echo "Searching checkbox implementation for press handlers..."
rg -C 5 "press" packages/components/checkbox/src/use-checkbox.ts
echo "Looking for usePress hook implementation..."
rg -C 5 "usePress" packages/components/checkbox/src/use-checkbox.ts
echo "Checking for touch event related aria attributes..."
rg "aria-(pressed|touchstart|touchend)" packages/components/checkbox/
Length of output: 3466
packages/components/switch/src/use-switch.ts (1)
158-162: LGTM!
The integration of usePress and management of isPressed state enhances the switch component's interaction handling.
packages/components/radio/src/radio.tsx (1)
23-23: Change is acceptable; ensures accessibility without VisuallyHidden.
The removal of the VisuallyHidden component in favor of styling the input element directly maintains accessibility while simplifying the DOM structure.
packages/components/checkbox/src/checkbox.tsx (1)
28-28: Verify accessibility after VisuallyHidden removal
While removing the VisuallyHidden wrapper might improve touch interactions, we need to ensure the input remains accessible to screen readers while being visually hidden.
Consider adding a CSS utility class for visually hidden elements that maintains accessibility:
.visually-hidden {
position: absolute;
width: 1px;
height: 1px;
padding: 0;
margin: -1px;
overflow: hidden;
clip: rect(0, 0, 0, 0);
white-space: nowrap;
border: 0;
}✅ Verification successful
Let me gather more information about the checkbox component and its styling to better understand how visual hiding is handled.
Let me check the useCheckbox hook implementation to understand how the styling and accessibility attributes are handled.
Let me check the theme implementation for the checkbox to understand how the input is styled.
Input is properly hidden while maintaining accessibility
The checkbox input is correctly implemented with proper accessibility considerations:
- The input is visually hidden using CSS positioning and opacity:
className: "absolute inset-0 opacity-0 z-50 cursor-pointer"- The input remains interactive and focusable due to:
- Maintaining its presence in the DOM
- Having
z-50to ensure it's clickable - Keeping
cursor-pointer
- The implementation uses React Aria's useCheckbox hook which provides proper ARIA attributes and keyboard interactions
The removal of VisuallyHidden wrapper is actually an improvement as it simplifies the implementation while maintaining accessibility through CSS.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CSS classes that might handle visual hiding
rg -g '*.{css,scss}' 'visually-hidden|sr-only|hidden'
# Check if the input receives necessary ARIA attributes
ast-grep --pattern 'input {...$_}'
Length of output: 390
Script:
#!/bin/bash
# Check the checkbox component's styling
rg -g '*.{css,scss,ts,tsx}' -C 5 'checkbox.*input'
# Look for any CSS-in-JS styling related to the checkbox input
ast-grep --pattern 'const $_ = styled($$$)input'
# Check if there's any style prop passed to input
ast-grep --pattern '<input style={$_}'
# Check the complete checkbox component implementation
cat packages/components/checkbox/src/checkbox.tsx
Length of output: 5946
Script:
#!/bin/bash
# Check the useCheckbox hook implementation
cat packages/components/checkbox/src/use-checkbox.ts
# Look for any styled components related to checkbox
rg -g '*.{ts,tsx}' -C 5 'styled.*checkbox'
# Check if there are any CSS modules
fd -e module.css -e module.scss . packages/components/checkbox/
Length of output: 10648
Script:
#!/bin/bash
# Check the checkbox theme implementation
cat packages/components/theme/src/components/checkbox.ts
# Look for any base styles that might handle input visibility
rg -g '*.{ts,tsx}' -C 5 'absolute.*opacity-0|opacity-0.*absolute'
Length of output: 17131
packages/components/switch/src/switch.tsx (1)
37-37: Ensure adequate touch target size
The switch component should maintain proper touch target dimensions (minimum 44x44px for iOS, 48x48px for Android) for optimal touch interaction.
Consider adding touch-specific styles:
.touch-target {
min-width: 48px;
min-height: 48px;
touch-action: manipulation;
}✅ Verification successful
Let me gather more specific information about the switch component's styles and dimensions.
Let me check the switch component's implementation more specifically.
Touch target size is adequately handled by the component
The switch component already implements proper touch target sizes through its theme configuration:
- Small (sm): 40px × 24px (w-10 h-6)
- Medium (md): 48px × 28px (w-12 h-7) - default size
- Large (lg): 56px × 32px (w-14 h-8)
The default medium size (48px × 28px) meets the recommended touch target dimensions for both iOS (44px) and Android (48px). Additionally, the component:
- Uses proper cursor and touch handling with
cursor-pointer touch-none - Implements tap highlight transparency for better mobile experience
- Includes proper focus management and keyboard interaction
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for touch-related styles and dimensions
rg -g '*.{css,scss}' 'min-(width|height)|touch-action'
# Look for existing touch event handlers
ast-grep --pattern 'onTouchStart|onTouchEnd|onClick'
Length of output: 161
Script:
#!/bin/bash
# Search for switch-specific styles and dimensions
rg -g '*.{css,scss,ts,tsx}' -A 5 'switch|Switch' --ignore-case
# Look for style-related props or CSS classes in the switch component
ast-grep --pattern 'className|style'
# Check if there are any size-related constants or utilities
rg -g '*.{ts,tsx}' 'size|dimension|width|height' --ignore-case
Length of output: 67339
Script:
#!/bin/bash
# Look for switch-specific styles and dimensions in the toggle theme
cat packages/core/theme/src/components/toggle.ts
# Check the switch implementation for any size-related props or styles
cat packages/components/switch/src/switch.tsx
Length of output: 6720
apps/docs/content/components/radio-group/custom-impl.raw.tsx (2)
21-21: Ensure proper touch target size with relative positioning
The addition of relative positioning is necessary for absolute positioning of the input, but we should verify the touch target size meets accessibility standards.
✅ Verification successful
Touch target size meets accessibility standards
The implementation includes sufficient padding and dimensions:
- Uses
p-4(16px padding) on all sides - Has a minimum width of 300px (
max-w-[300px]) - Full element is clickable due to
inset-0on the input - Includes proper spacing with
gap-4between elements
The touch target implementation is actually more accessible than a standard radio button as it makes the entire container clickable while maintaining proper visual feedback through hover states and borders.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any existing touch target size specifications
rg -i "touch-target|tap-target" .
Length of output: 35
Script:
#!/bin/bash
# Let's check the actual dimensions and spacing classes used in the radio implementation
rg -A 5 -B 5 "relative group inline-flex" .
# Also check for any size-related classes or styles
ast-grep --pattern 'className={`$$$`}'
Length of output: 2123
26-26: Verify input element accessibility
The replacement of VisuallyHidden with direct styling maintains functionality but requires verification of:
- Screen reader compatibility
- Focus visibility
- Touch event propagation
Consider adding explicit ARIA attributes to enhance screen reader compatibility:
-<input {...getInputProps()} className="absolute inset-0 z-50 cursor-pointer opacity-0" />
+<input {...getInputProps()} className="absolute inset-0 z-50 cursor-pointer opacity-0" aria-hidden="false" />apps/docs/content/components/checkbox/custom-implementation.raw.jsx (1)
53-53:
Verify touch event propagation with Chip component
The absolute positioning of the input over the Chip component could affect touch event handling. We need to ensure that:
- Touch events properly trigger both the input and Chip state changes
- No double-triggering occurs
- The z-index of 50 doesn't interfere with other interactive elements
Consider adding event stopping to prevent propagation issues:
-<input {...getInputProps()} className="absolute inset-0 z-50 cursor-pointer opacity-0" />
+<input {...getInputProps()}
+ className="absolute inset-0 z-50 cursor-pointer opacity-0"
+ onClick={(e) => e.stopPropagation()}
/>apps/docs/content/components/switch/custom-impl.raw.jsx (1)
48-48: Ensure consistent behavior across all form controls
The implementation follows the same pattern as radio and checkbox, but we should ensure consistent behavior:
- Touch event handling
- Focus state management
- Icon switching animation
Consider extracting the common input styling to a shared utility:
const visuallyHiddenInputClass = "absolute inset-0 z-50 cursor-pointer opacity-0";This would ensure consistency across all form control implementations and make future updates easier to maintain.
apps/docs/content/components/switch/custom-impl.raw.tsx (1)
49-49: Verify touch event handling with the new input implementation
The direct input element with z-50 ensures it remains interactive while being visually hidden. However, we should verify that this doesn't interfere with other touch interactions.
✅ Verification successful
No z-index conflicts found with the input implementation
The search results show that z-50 is consistently used across similar interactive form elements (checkbox, radio, switch) and UI components that need to be on top. The pattern of using z-50 with opacity-0 for accessible but visually hidden inputs is an established pattern in the codebase, and there are no higher z-index values that would interfere with the touch interactions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential z-index conflicts in the codebase
rg -g '*.{css,scss,tsx,jsx}' 'z-[5-9][0-9]|z-\[([5-9][0-9]|100)\]' --type-add 'css:*.{css,scss}'
Length of output: 3230
packages/components/switch/stories/switch.stories.tsx (1)
114-114: LGTM! Consistent implementation across components
The implementation matches the approach used in custom implementation, ensuring consistent behavior across different usage patterns.
packages/components/radio/stories/radio.stories.tsx (1)
423-423: LGTM: Improved accessibility and touch interaction
The changes properly handle input positioning and touch events:
- Added relative positioning to the container
- Properly positioned input element with z-index for touch interaction
Also applies to: 429-429
|
Closing this in favor of this PR #4311 |
📝 Description
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
VisuallyHiddencomponent from Checkbox, Switch, and Radio implementations.Tests
Documentation