Fix for the radio button not working with onChange #287
Conversation
As suggested in the comment to the merged PR (ref: preactjs#247) the event normalisation has broken the support for onChange on radio buttons. This is a potential fix
| @@ -337,7 +337,7 @@ function applyEventNormalization({ nodeName, attributes }) { | |||
| } | |||
| if (props.onchange) { | |||
| nodeName = nodeName.toLowerCase(); | |||
| let attr = nodeName==='input' && String(attributes.type).toLowerCase()==='checkbox' ? 'onclick' : 'oninput', | |||
| let attr = nodeName==='input' && ['checkbox', 'radio'].indexOf(String(attributes.type).toLowerCase()) > -1 ? 'onclick' : 'oninput', | |||
There was a problem hiding this comment.
Looks great! Any chance we could shorten this to the technique used here?
let attr = nodeName==='input' && attributes.type.match(/^che|rad/i) ? 'onclick' : 'oninput',There was a problem hiding this comment.
yep, it makes sense, I will make the change
There was a problem hiding this comment.
just one thing: do you want me to lose the String(...).toLowerCase(): it was in the original, that's why I kept it
There was a problem hiding this comment.
The PR is updated without it anyway, if you need it back let me know
There was a problem hiding this comment.
We might need the String() wrap, the regex handles case insensitivity.
|
(left this inline but it got collapsed by github haha) Good catch on that, it would have bailed for let attr = nodeName==='input' && /^che|rad/i.test(attributes.type) ? 'onclick' : 'oninput',we're good to merge once that's in place :) I might add a few tests for posterity post-merge. |
|
The PR is updated with the changes, thanks for the speedy process |
|
thanks for the speedy fix! |
|
@developit why delete onclick for radio now? |
As suggested in the comment to the merged PR (ref: #247) the event normalisation has broken the support for onChange on radio buttons. This is a potential fix