Update findFormItem so it can return checked radio buttons #2734

Merged
merged 1 commit into from Feb 25, 2017

Conversation

2 participants
@clguimanMSFT
Contributor

clguimanMSFT commented Feb 3, 2017

This is useful when you want to get the value of a radio button in the validate method of f:validateButton

@@ -350,7 +350,14 @@ function findNext(src,filter) {
function findFormItem(src,name,directionF) {
var name2 = "_."+name; // handles <textbox field="..." /> notation silently
- return directionF(src,function(e){ return (e.tagName=="INPUT" || e.tagName=="TEXTAREA" || e.tagName=="SELECT") && (e.name==name || e.name==name2); });
+ return directionF(src,function(e){
+ if (e.tagName == "INPUT" && e.type=="radio" && e.checked==true) {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Feb 4, 2017

Member

And no name in the condition?

@oleg-nenashev

oleg-nenashev Feb 4, 2017

Member

And no name in the condition?

This comment has been minimized.

@clguimanMSFT

clguimanMSFT Feb 4, 2017

Contributor

the name will not match here, I need to parse out the 'removeme'. I'm doing that inside the if

@clguimanMSFT

clguimanMSFT Feb 4, 2017

Contributor

the name will not match here, I need to parse out the 'removeme'. I'm doing that inside the if

+ return directionF(src,function(e){
+ if (e.tagName == "INPUT" && e.type=="radio" && e.checked==true) {
+ var r = 0;
+ while (e.name.substring(r,r+8)=='removeme') //radio buttons have must be unique in repeatable blocks so name is prefixed

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Feb 4, 2017

Member

I doubt it is the most effective method to find the value within the string

@oleg-nenashev

oleg-nenashev Feb 4, 2017

Member

I doubt it is the most effective method to find the value within the string

This comment has been minimized.

@clguimanMSFT

clguimanMSFT Feb 4, 2017

Contributor

I wanted to use the same code as in buildFormTree:

@clguimanMSFT

clguimanMSFT Feb 4, 2017

Contributor

I wanted to use the same code as in buildFormTree:

@oleg-nenashev

LGTM. A test would be useful on the other hand

@clguimanMSFT

This comment has been minimized.

Show comment
Hide comment
@clguimanMSFT

clguimanMSFT Feb 6, 2017

Contributor

I'm new to the project, can you please point me to where I should add my tests? I wasn't aware there were js unit tests

Contributor

clguimanMSFT commented Feb 6, 2017

I'm new to the project, can you please point me to where I should add my tests? I wasn't aware there were js unit tests

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Feb 12, 2017

Member

@clguimanMSFT In this case it is rather a subject for Jenkins Acceptance Test Harness: https://github.com/jenkinsci/acceptance-test-harness/ .

Member

oleg-nenashev commented Feb 12, 2017

@clguimanMSFT In this case it is rather a subject for Jenkins Acceptance Test Harness: https://github.com/jenkinsci/acceptance-test-harness/ .

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Feb 12, 2017

Member

@jenkinsci/code-reviewers Would appreciate additional +1 from a JS guru

Member

oleg-nenashev commented Feb 12, 2017

@jenkinsci/code-reviewers Would appreciate additional +1 from a JS guru

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Feb 17, 2017

Member

@jenkinsci/code-reviewers Ping. @daniel-beck Are you fine with the merge tomorrow?

Member

oleg-nenashev commented Feb 17, 2017

@jenkinsci/code-reviewers Ping. @daniel-beck Are you fine with the merge tomorrow?

@oleg-nenashev oleg-nenashev self-assigned this Feb 17, 2017

@oleg-nenashev oleg-nenashev merged commit 0713bf0 into jenkinsci:master Feb 25, 2017

2 checks passed

Jenkins This pull request looks good
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment