-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(MenuItemSelectable): remove duplicate onChange #17967
fix(MenuItemSelectable): remove duplicate onChange #17967
Conversation
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for v11-carbon-react ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #17967 +/- ##
==========================================
+ Coverage 81.80% 81.82% +0.02%
==========================================
Files 404 404
Lines 14088 14086 -2
Branches 4409 4379 -30
==========================================
+ Hits 11524 11526 +2
+ Misses 2401 2397 -4
Partials 163 163 ☔ View full report in Codecov by Sentry. |
This is the same issue and fix that was previously addressed for `MenuItemRadioGroup` in carbon-design-system#17736. Also added a test to cover this case, including checking the value passed to the `onChange` prop as the redundant `onChange` being removed here was passing the SyntheticEvent instead of the expected boolean.
e6f8f77
to
2f8173e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing that! LGTM
Hi folks, is there anything else needed to get this merged? Looks like all checks are passing. |
Looks like it's good to go! Added to the merge queue |
664408d
Closes #17968
This is the same issue and fix that was previously addressed for
MenuItemRadioGroup
in #17736.Also added a test to cover this case, including checking the value passed to the
onChange
prop as the redundantonChange
being removed here was passing the SyntheticEvent instead of the expected boolean.Changelog
Changed
onChange
is only called onceTesting / Reviewing
Unit test added to cover the expected behaviour.