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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-55787] Switch labels from entry to checkbox #3921

Merged
merged 3 commits into from
May 24, 2019
Merged

[JENKINS-55787] Switch labels from entry to checkbox #3921

merged 3 commits into from
May 24, 2019

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Mar 4, 2019 •
edited
Loading

See JENKINS-55787.

This is split from #3895 -- it's independent

Proposed changelog entries

  • Switch labels from entry to checkbox

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@batmat batmat closed this Mar 5, 2019
@batmat batmat reopened this Mar 5, 2019
@oleg-nenashev oleg-nenashev added the proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it label Mar 6, 2019
@daniel-beck
Copy link
Member

@oleg-nenashev Why the label?

@oleg-nenashev
Copy link
Member

My bad. I believe I messed up something while editing label descriptions

@oleg-nenashev oleg-nenashev removed the proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it label Mar 6, 2019
varyvol
varyvol reviewed Mar 25, 2019
View reviewed changes
test/src/test/java/hudson/model/ParametersTest.java
@@ -73,7 +73,7 @@ public void parameterTypes() throws Exception {
System.out.println(o);
HtmlCheckBoxInput booleanParameterInput = (HtmlCheckBoxInput) o;
assertEquals(true, booleanParameterInput.isChecked());
assertEquals("boolean", ((HtmlElement) DomNodeUtil.selectSingleNode(element, "td[@class='setting-name']")).getTextContent());
assertEquals("boolean", ((HtmlElement) DomNodeUtil.selectSingleNode(element, "td[@class='setting-main']")).getTextContent());
Copy link

Choose a reason for hiding this comment

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

Why does it happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the old style, you have something roughly like:

setting-name setting-main
Label [x]

In the new, it's closer to:

setting-name setting-main
~ [x] Label

@varyvol
Copy link

varyvol commented Mar 26, 2019

@jsoref does something change in the UI?

@jsoref
Copy link
Contributor Author

jsoref commented Mar 26, 2019

@varyvol: yes, as shown in the issue description

@varyvol
Copy link

varyvol commented Mar 26, 2019

Did you have then the opportunity to run ATH tests?

@Wadeck Wadeck added the web-ui The PR includes WebUI changes which may need special expertise label Mar 26, 2019
oleg-nenashev
oleg-nenashev approved these changes May 2, 2019
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Weak +1. I have not checked all the forms, but it looks reasonable in all cases I see.

@varyvol ATH should not be affected if they query forms using the common API. I do not think there is a need to run it here

@oleg-nenashev oleg-nenashev requested a review from varyvol May 2, 2019 14:31
@oleg-nenashev
Copy link
Member

@jenkinsci/core Few extra reviews will be appreciated

@jglick
Copy link
Member

jglick commented May 6, 2019 •
edited
Loading

Of course plugins should not assume particular core resource paths exist.

@jsoref
Copy link
Contributor Author

jsoref commented May 6, 2019

@jglick : https://issues.jenkins-ci.org/browse/JENKINS-57346 ~24 plugins would need to be cleaned up to properly address your concern. And in doing so, that will mean that they'll each essentially be forking content... I'm not 100% certain that's the right choice, but...

@oleg-nenashev oleg-nenashev requested review from dwnusbaum and jglick and removed request for dwnusbaum May 8, 2019 16:42
@oleg-nenashev oleg-nenashev added the needs-more-reviews Complex change, which would benefit from more eyes label May 10, 2019
@oleg-nenashev
Copy link
Member

Still needs more approvals from @jenkinsci/core

@jsoref
Copy link
Contributor Author

jsoref commented May 14, 2019

@dwnusbaum, @jglick: updated...

dwnusbaum
dwnusbaum approved these changes May 14, 2019
View reviewed changes
Copy link
Member

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

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

I don't see any obvious problems and the ATH seems to be fine with the changes, so a weak +1 from me.

@oleg-nenashev oleg-nenashev added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels May 16, 2019
@oleg-nenashev
Copy link
Member

Probably we should wait for the feedback from @jglick before merging

@oleg-nenashev
Copy link
Member

bump @jglick . I tentatively plan to merge it on Thursday if there is no negative feedback

@oleg-nenashev oleg-nenashev merged commit 01a3c57 into jenkinsci:master May 24, 2019
@jsoref jsoref deleted the JENKINS-55787 branch May 26, 2019 02:36
@jsoref jsoref mentioned this pull request May 26, 2019
Merged
4 tasks
@daniel-beck
Copy link
Member

https://issues.jenkins-ci.org/browse/JENKINS-59388 reports this as a bug.

@daniel-beck daniel-beck mentioned this pull request Mar 29, 2020
Merged
5 tasks
@daniel-beck daniel-beck mentioned this pull request Jan 24, 2022
Merged
10 tasks
daniel-beck
daniel-beck reviewed Oct 13, 2023
View reviewed changes
core/src/main/resources/hudson/model/BooleanParameterValue/value.jelly
Comment on lines +30 to +31
<f:checkbox title="${h.escape(it.name)}" description="${it.formattedDescription}" name="value" checked="${it.value}" readonly="true" />
Copy link
Member

Choose a reason for hiding this comment

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

Caused https://issues.jenkins.io/browse/JENKINS-72179

@daniel-beck daniel-beck mentioned this pull request Oct 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daniel-beck daniel-beck daniel-beck left review comments

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

@dwnusbaum dwnusbaum dwnusbaum approved these changes

@varyvol varyvol Awaiting requested review from varyvol

@jglick jglick Awaiting requested review from jglick

Assignees
No one assigned
Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@jsoref @daniel-beck @oleg-nenashev @varyvol @jglick @dwnusbaum @batmat @Wadeck