fix: resolve IconButton onClick handler issue preventing clicks #7553
+1
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The IconButton component was conditionally setting the onClick handler based on the disabled state, which could prevent clicks from being handled properly when onClick was undefined. The button now always passes through the onClick handler and relies on the disabled attribute to prevent clicks when disabled. Fixes #7552
dosubot
bot
added
size:XS
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| )} | ||
| disabled={disabled} | ||
| onClick={!disabled ? onClick : undefined} | ||
| onClick={onClick} |
Contributor
Author
There was a problem hiding this comment.
The fix looks correct - removing the conditional onClick handler and relying on the native button's disabled attribute is the right approach. This simplifies the code and fixes the issue where buttons weren't responding to clicks.
A few observations for future consideration:
- Would it be worth adding tests for this component to prevent regression?
- I noticed there's another IconButton in
webview-ui/src/components/common/IconButton.tsx- is this duplication intentional? - Consider adding a comment explaining why conditional onClick isn't needed with the disabled attribute.
hannesrudolph
added
the
Issue/PR - Triage
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
github-project-automation
bot
moved this from PR [Needs Prelim Review]
to Done
in Roo Code Roadmap
|
This problem is still here in version 3.28.14... |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR attempts to address Issue #7552 where the settings gear icon and other buttons were not responding to clicks in a freshly installed extension.
Problem
The IconButton component was conditionally setting the onClick handler based on the disabled state:
This pattern could prevent clicks from being handled properly when the button was not explicitly disabled.
Solution
The fix removes the conditional onClick attachment and relies on the native HTML button behavior where
disabled={true}prevents clicks at the browser level:Testing
Related Issue
Feedback and guidance are welcome!
Important
Fixes
onClickhandler inIconButtonto always attach, relying on nativedisabledbehavior to prevent clicks.onClickhandler inIconButtoncomponent inIconButton.tsxto always attachonClick, relying on nativedisabledbehavior to prevent clicks.This description was created by
for ac5775c. You can customize this summary. It will automatically update as commits are pushed.