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

Update unochoice.js #2

Merged
merged 1 commit into from
Jul 22, 2015
Merged

Update unochoice.js #2

merged 1 commit into from
Jul 22, 2015

Conversation

jhosmer
Copy link

@jhosmer jhosmer commented Jul 21, 2015

Fixed a bug where they were calling the 'value' attribute on text input elements (which only returns the default value, not the current live value). Changed .attr('value') to the proper call: .val(). Also, the 'checked' attribute simply has to exist for the element to be considered checked, so it is incorrect to only compare it with checked="checked". Therefore, the following change was made: If attr('checked') is not undefined, then consider the element checked.

Fixed a bug where they were calling the 'value' attribute on text input elements (which only returns the default value, not the current live value).  Changed `.attr('value')` to the proper call: `.val()`.  Also, the 'checked' attribute simply has to exist for the element to be considered checked, so it is incorrect to only compare it with `checked="checked"`.  Therefore, the following change was made: If attr('checked') is not undefined, then consider the element checked.
@jenkinsadmin
Copy link
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@kinow
Copy link
Member

kinow commented Jul 22, 2015

Hi @jhosmer!

Your changes make sense. I'll give it a try locally and will merge if I find no errors. But since I'm not a JavaScript developer, it is very likely that I've introduced a few bugs like this in the code :-)

Thanks a lot for your contribution.
Bruno

@kinow
Copy link
Member

kinow commented Jul 22, 2015

Had a look at the following resources to educate myself

http://stackoverflow.com/questions/8312820/jquery-val-vs-attrvalue
http://stackoverflow.com/questions/4837133/whats-the-difference-between-jquery-val-and-attrvalue
https://teamtreehouse.com/forum/why-use-val-instead-of-attrvalue

And tested locally. Everything looks good, quite tricky to write tests for that without some further thinking, so I'll merge the pull request and include it in the next release anyway :-)

kinow added a commit that referenced this pull request Jul 22, 2015
@kinow kinow merged commit 00a8c29 into jenkinsci:master Jul 22, 2015
@kinow
Copy link
Member

kinow commented Jul 22, 2015

Merged. Thank you! Next release planned as soon as pull request 1 has been reviewed (hopefully tonight, or tomorrow).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants