-
Notifications
You must be signed in to change notification settings - Fork 10
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
Set radio button checked on host click #85
Conversation
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.
Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @samiheikki)
src/vaadin-radio-button.html, line 211 at r1 (raw file):
e.preventDefault(); this.checked = true; this.setAttribute('active', '');
Setting the active
state on click is not appropriate.
The active
state should only be set during the click (set on mousedown
, removed on mouseup
), not after.
f7833c9
to
f366727
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.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @platosha and @samiheikki)
src/vaadin-radio-button.html, line 211 at r1 (raw file):
Previously, platosha (Anton Platonov) wrote…
Setting the
active
state on click is not appropriate.The
active
state should only be set during the click (set onmousedown
, removed onmouseup
), not after.
Good point. Removed it.
src/vaadin-radio-button.html, line 208 at r2 (raw file):
Can you add a test for this condition also? (Shouldn't set a disabled host checked on click) |
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.
Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @samiheikki)
f366727
to
e8a3773
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.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @platosha and @tomivirkki)
src/vaadin-radio-button.html, line 208 at r2 (raw file):
Previously, tomivirkki (Tomi Virkki) wrote…
Can you add a test for this condition also? (Shouldn't set a disabled host checked on click)
Done.
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.
Reviewed 1 of 1 files at r2, 1 of 1 files at r3.
Reviewable status:complete! all files reviewed, all discussions resolved
Fixes #84
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)