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

Checkbox [fix]: prevent default onkeypress action #817

Merged
merged 1 commit into from
Mar 12, 2021
Merged

Conversation

eliobricenov
Copy link
Contributor

Description

Given that the <Checkbox/> component renders a <span> element with a <button> element as a parent, by default, the Enter key triggers the keypress event.

This PR disables the default action on the keypress event that triggers the click event later on.

Note: the checkbox can still be checked with the Space key thanks to the role attribute.

Resolves #626

@eliobricenov eliobricenov self-assigned this Feb 17, 2021
@auto-assign auto-assign bot requested a review from bpierre February 17, 2021 04:07
@welcome
Copy link

welcome bot commented Feb 17, 2021

Thanks for opening this pull request! Someone will review it soon 🔍

@CLAassistant
Copy link

CLAassistant commented Feb 17, 2021

CLA assistant check
All committers have signed the CLA.

@eliobricenov eliobricenov changed the title fix: prevent default onkeypress action Checkbox [fix]: prevent default onkeypress action Feb 17, 2021
@eliobricenov eliobricenov requested review from nivida and removed request for bpierre and sohkai February 19, 2021 16:26
Copy link

@nivida nivida left a comment

Choose a reason for hiding this comment

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

The change itself looks good. But was it required to update the dependencies as for example rollup/plugin-node-resolve? I especially ask this because it is a major version increase.

Overall we can say that changes per PR should be kept at a minimum. This means the description of a PR and the related issue/task defines the scope of it and any further changes have to be done in a separate PR with a related task.

@eliobricenov
Copy link
Contributor Author

The change itself looks good. But was it required to update the dependencies as for example rollup/plugin-node-resolve? I especially ask this because it is a major version increase.

Overall we can say that changes per PR should be kept at a minimum. This means the description of a PR and the related issue/task defines the scope of it and any further changes have to be done in a separate PR with a related task.

@nivida Those changes were the result of running yarn. I figured they were not necessary but the Travis build was failing without them. Either way, I already removed them because I also think they're out of scope for this PR.

@aragon aragon deleted a comment from Horus78 Mar 12, 2021
@nivida
Copy link

nivida commented Mar 12, 2021

@eliobricenov Thanks for the insights! :) Feel free to create a task on Linear related to the required dependency updates and add it to the circle. I think it will be a quick thing for you to open a PR for and we would better see the work you already have done :-)

@nivida nivida merged commit 9947758 into master Mar 12, 2021
@welcome
Copy link

welcome bot commented Mar 12, 2021

Congrats on merging your first pull request! Aragon is proud of you 🦅 Eagle gif

@nivida nivida deleted the aragon-626 branch March 12, 2021 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checkbox, Radio: avoid toggling on 'enter'
3 participants