Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Form checker enhancements for radio #10081

Merged
merged 1 commit into from
Dec 22, 2024

Conversation

timja
Copy link
Member

@timja timja commented Dec 19, 2024

Basically the same as #8832 except for general form validation and not f:validateButton

Previously it only found the last radio button, it tried to find the checked one but then just fell through to the general code 😢. Updates didn't work either it just bound to the returned elements and not all of the radio buttons

Testing done

Tested in Azure VM agents as part of: jenkinsci/azure-vm-agents-plugin#604

Proposed changelog entries

  • Form validation that depends on radio buttons now finds the selected one and not the last one

Proposed upgrade guidelines

N/A

Submitter checklist

Preview Give feedback

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

Preview Give feedback

@timja timja added the bug For changelog: Minor bug. Will be listed after features label Dec 19, 2024
@@ -536,21 +536,24 @@ function findNext(src, filter) {
}

function findFormItem(src, name, directionF) {
var name2 = "_." + name; // handles <textbox field="..." /> notation silently
const name2 = "_." + name; // handles <textbox field="..." /> notation silently
Copy link
Member Author

Choose a reason for hiding this comment

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

some minor tidyup in this function to use === and const / let as well

//radio buttons have must be unique in repeatable blocks so name is prefixed
r = e.name.indexOf("_", r + 8) + 1;
if (e.tagName === "INPUT" && e.type === "radio" ) {
if (e.checked === true) {
Copy link
Member Author

Choose a reason for hiding this comment

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

moved checked out of if so it doesn't fallthrough to the return on 552

}
return name == e.name.substring(r);
return false
Copy link
Member Author

Choose a reason for hiding this comment

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

if not checked continue and see if we can find a checked one

if (c.tagName === 'INPUT' && c.type === "radio") {
document.querySelectorAll(`input[name='${c.name}'][type='radio']`)
.forEach(element => {
element.addEventListener("change", checker.bind(e));
Copy link
Member Author

Choose a reason for hiding this comment

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

bind to all of the radio buttons so that when someone clicks on one form validation is updated

@timja timja marked this pull request as ready for review December 19, 2024 14:51
@timja timja requested a review from a team December 19, 2024 14:51
@timja timja added the web-ui The PR includes WebUI changes which may need special expertise label Dec 19, 2024
@timja timja requested a review from a team December 20, 2024 09:02
@timja
Copy link
Member Author

timja commented Dec 20, 2024

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Dec 20, 2024
@krisstern krisstern merged commit 6a86f2f into jenkinsci:master Dec 22, 2024
15 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug For changelog: Minor bug. Will be listed after features ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants