fix: Add onPress function/handler to web Select trigger #65
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis change updates the press event handling in the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant T as Trigger Component
participant O as onOpenChange Handler
participant P as onPress Callback
U->>+T: Triggers press event
T-->>T: Check if component is disabled
alt Component enabled
T->>+O: Toggle open state
O-->>-T: Open state toggled
T->>+P: Call onPress with event
P-->>-T: Callback executed
else Component disabled
T-->>U: No action taken (disabled)
end
Poem
✨ Finishing Touches
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:
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
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 (1)
packages/select/src/select.web.tsx (1)
114-155: Consider adding documentation for the new onPress prop.The implementation is correct, but it would be helpful to add JSDoc comments for the new
onPressprop to help consumers understand its purpose and usage, similar to how other components in this file are documented.You could add something like:
const Trigger = React.forwardRef<TriggerRef, TriggerProps>( /** * Trigger component for the Select * @param onPress - Optional callback when the trigger is pressed */ ({ asChild, onPress: onPressProp, role: _role, disabled, ...props }, ref) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/select/src/select.web.tsx(3 hunks)
🔇 Additional comments (4)
packages/select/src/select.web.tsx (4)
9-9: Added GestureResponderEvent type import correctly.Good addition of the
GestureResponderEventtype import from react-native, which is needed for proper type checking of the event parameter in the newonPressfunction.
115-115: Added onPress prop to Trigger component correctly.Good implementation of adding the
onPressprop to the Trigger component's props, allowing consumers to provide their own press handler while maintaining the component's core functionality.
117-121: Well-implemented onPress handler function.The
onPressfunction implementation follows good practices:
- It first checks if the component is disabled before proceeding
- It handles the core functionality (toggling open state)
- It then calls the custom handler if provided
This approach ensures the component's primary behavior works as expected while allowing for extensibility.
146-152: Updated Component JSX to use the new onPress handler.The Component JSX has been properly updated to use the new
onPressfunction, ensuring that both the core functionality and any custom behavior will be executed when the component is pressed.
| function onPress(ev: GestureResponderEvent) { | ||
| if (disabled) return; | ||
| onOpenChange(!open); | ||
| onPressProp?.(ev); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider unit testing the new onPress functionality.
The implementation is good, but you might want to verify:
- The default toggle behavior works as expected
- The custom onPress handler is called when provided
- Neither function executes when the component is disabled
🏁 Script executed:
#!/bin/bash
# Check if there are any unit tests for the Trigger component
echo "Searching for existing tests..."
find . -type f -name "*.test.tsx" -o -name "*.spec.tsx" | xargs grep -l "Trigger" || echo "No test files found mentioning Trigger"
echo "Checking if onPress is tested in any files..."
find . -type f -name "*.test.tsx" -o -name "*.spec.tsx" | xargs grep -l "onPress.*Trigger" || echo "No test files found testing onPress on Trigger"Length of output: 538
Action Required: Add Unit Tests for onPress Functionality
The implementation in packages/select/src/select.web.tsx looks correct. However, our verification indicates that there are no existing tests covering the onPress functionality. Please add unit tests to ensure:
- The default toggle behavior (
onOpenChange(!open)) works as expected. - The custom
onPressPropcallback is invoked when provided. - Neither action is executed when the component is disabled.
|
This is with reference to issue #66 |
Summary by CodeRabbit