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

JENKINS-55787 Switch labels from entry to checkbox #860

Merged
merged 3 commits into from
Apr 5, 2020

Conversation

timja
Copy link
Member

@timja timja commented Mar 29, 2020

JENKINS-55787 - Switch labels from entry to checkbox

2.227-SNAPSHOT

image

2.138.4

image

You can also test via reverting my revert jenkins core version commit

ref: jenkinsci/jenkins#3895, cc @MarkEWaite @jsoref

Checklist

  • I have read the CONTRIBUTING doc
  • I have referenced the Jira issue related to my changes in one or more commit messages
  • Unit tests pass locally with my changes
  • I have added documentation as necessary
  • No Javadoc warnings were introduced with my changes
  • No spotbugs warnings were introduced with my changes
  • I have interactively tested my changes

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

pom.xml Outdated Show resolved Hide resolved
@jsoref
Copy link
Contributor

jsoref commented Mar 29, 2020

This supercedes #676

@MarkEWaite MarkEWaite assigned MarkEWaite and romenrg and unassigned MarkEWaite Apr 4, 2020
@romenrg
Copy link
Member

romenrg commented Apr 5, 2020

I'm not very familiar with this, but here are my two cents:

Just tested this locally and it seems to work fine, although my UI looks slightly different to what I see in the PR description by @timja (I'm probably missing something):

Screen Shot 2020-04-05 at 11 41 14

From @jsoref comment and linked PR I understand the idea is to have attributes associated to the checkbox rather than to the entry.

Taking a look at the resulting html, I see this code:
Screen Shot 2020-04-05 at 11 48 05

Which seems to come from this change:
Screen Shot 2020-04-05 at 11 51 07

I wonder if this is the expected result. Maybe it is, just not sure, but I still see the title associated to the label instead of it being associated to the checkbox. Or maybe I'm doing something wrong.

Also, in the source code I still see the "field" attribute associated to the entry, while in Joe's PR I see the "field" attributes switched from the entry to the checkbox, along with all the others. Is there a reason to keep the field attribute in the entry in these cases?

@timja
Copy link
Member Author

timja commented Apr 5, 2020

There’s been a few commits to the PR since the screenshot was taken, help icon alignment has been fixed and related nested item layout has been adjusted.

I’m not sure if the field attribute location matters.

That would be why it looks different

@MarkEWaite MarkEWaite added the bugfix Fixes a bug - used by Release Drafter label Apr 5, 2020
@MarkEWaite MarkEWaite merged commit c1dda1e into jenkinsci:master Apr 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes a bug - used by Release Drafter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants