Skip to content
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: Native radio input firing onChange callback twice on click #791

Merged
merged 2 commits into from Nov 26, 2020

Conversation

david-szabo97
Copy link
Contributor

Fixes #790

change event is fired for native radio inputs by the browser. Therefore we don't need to fire it when we click on a native radio.

How to test?

Regression test included

Does this PR introduce breaking changes?

No

@github-actions
Copy link
Contributor

Size Change: +4 B (0%)

Total Size: 256 kB

Filename Size Change
packages/reakit/dist/reakit.min.js 35.8 kB +4 B (0%)
ℹ️ View Unchanged
Filename Size Change
packages/reakit-playground/dist/reakit-playground.min.js 187 kB 0 B
packages/reakit-system-bootstrap/dist/reakit-system-bootstrap.min.js 19.1 kB 0 B
packages/reakit-system-palette/dist/reakit-system-palette.min.js 8.85 kB 0 B
packages/reakit-system/dist/reakit-system.min.js 2.45 kB 0 B
packages/reakit-utils/dist/reakit-utils.min.js 3.38 kB 0 B

compressed-size-action

@ariakit-bot
Copy link

Deploy preview for reakit ready!

Built with commit 065887f

https://deploy-preview-791--reakit.netlify.app

@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 065887f:

Sandbox Source
Reakit Configuration
React PlayGround (forked) Issue #790

@codecov
Copy link

codecov bot commented Nov 26, 2020

Codecov Report

Merging #791 (065887f) into master (57b1238) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #791   +/-   ##
=======================================
  Coverage   95.44%   95.44%           
=======================================
  Files         229      229           
  Lines        3493     3494    +1     
  Branches      914      950   +36     
=======================================
+ Hits         3334     3335    +1     
  Misses        158      158           
  Partials        1        1           
Impacted Files Coverage Δ
packages/reakit/src/Radio/Radio.ts 96.77% <100.00%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 57b1238...065887f. Read the comment docs.

Copy link
Member

@diegohaz diegohaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome @david-szabo97! Thanks a lot!

I wonder if we can force React to fire this event by using something similar to this so we don't need to call the onChange prop ourselves: https://github.com/reakit/reakit/blob/57b1238e714bb4dda3984d62268313c180fcc6a8/packages/reakit/src/Composite/__utils/setTextFieldValue.ts#L1-L15

With that, React will fire the onChange event on text inputs, but maybe there's a similar solution for non-native radio and checkbox.

@diegohaz diegohaz merged commit 6a436bb into master Nov 26, 2020
@diegohaz diegohaz deleted the fix/native-radio-firing-onchange-twice branch November 26, 2020 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Radio button onChange handler fires twice
4 participants