fix: [button-group] fixed the bug that multiple clicks on the same button would trigger multiple events #2590
Conversation
…tton would trigger multiple events
WalkthroughThe changes in this pull request focus on modifying the Changes
Possibly related PRs
Suggested labels
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
CodeRabbit Configuration File (
|
zzcr
changed the title
![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 (2)
packages/renderless/src/button-group/index.ts (2)
25-28: Add test cases to verify the fix.Please add test cases to verify:
- Multiple clicks on the same button trigger only one event
- Clicks on different buttons still work as expected
- Disabled state handling remains correct
Would you like me to help generate the test cases for these scenarios?
Line range hint
14-21: Consider applying similar protection to handleChange.The
handleChangefunction might benefit from similar value-change protection to maintain consistency with the click handler's behavior.Here's a suggested improvement:
export const handleChange = ({ emit, state }: Pick<IButtonGroupRenderlessParams, 'emit' | 'state'>) => (): void => { - if (!state.disabled) { + if (!state.disabled && state.previousValue !== state.value) { + state.previousValue = state.value emit('change', state.value) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/renderless/src/button-group/index.ts(1 hunks)
🔇 Additional comments (2)
packages/renderless/src/button-group/index.ts (2)
25-28: LGTM! The fix correctly prevents redundant events.
The additional condition state.value !== node[props.valueField] effectively prevents multiple events from being triggered when clicking the same button repeatedly.
25-28: Verify impact on existing usage patterns.
Let's verify that this change doesn't affect other components or existing usage patterns.
✅ Verification successful
Based on the extensive search results, I can now generate the final response:
The disabled state check is properly implemented and consistent with usage patterns
The code change adds a disabled state check before updating the value, which is consistent with:
- The component's usage patterns across the codebase, where button-group is used in forms and other components with disabled states
- The existing test files and examples that demonstrate disabled state handling
- The component's implementation in both PC and mobile versions
The change ensures that value updates don't occur when either:
- The overall button-group is disabled (state.disabled)
- The individual button node is disabled (node.disabled)
This is the correct behavior as it prevents value changes when the component or its items are in a disabled state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential event handlers in button-group components
echo "Checking for other button-group event handlers:"
rg -t ts "emit\(" packages/renderless/src/button-group/
# Search for button-group usage patterns
echo "Checking button-group usage patterns:"
rg -t vue "@click" -A 5 -B 5 packages/vue/src/button-group/
Length of output: 597
Script:
#!/bin/bash
# Search for button-group usage patterns with a more generic approach
echo "Checking button-group usage patterns in all files:"
rg "button-group" -A 5 -B 5
# Check for any test files related to button-group
echo "Checking for button-group test files:"
fd "button.*test" -t f
# Look for component documentation or examples
echo "Checking for button-group documentation or examples:"
fd "button.*md" -t f
Length of output: 66744
…tton would trigger multiple events
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