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-56109] Change Jenkins configuration UI from tables to divs #3895

Merged
merged 134 commits into from
Oct 23, 2020

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Feb 12, 2019

See JENKINS-56109.

Initially discussed in #3859

Test this

Screenshots

Configure

image

Configure mobile

image

Nested elements association

image

Configure pipeline

image

Configure freestyle

image

Current status:

Needs reviews, also see: https://groups.google.com/d/msg/jenkinsci-dev/MpJMqa1Yc9A/Vex1yix8AAAJ

Proposed changelog entries

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
  • UI Acceptance test / changes necessary
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@daniel-beck

Notes:

List of plugins provided by Daniel Beck which have jelly files that contain <td but not <table, indicating a reliance on existing structures at least in config.jelly and global.jelly files https://gist.github.com/daniel-beck/ef4a6f38d78014343a0ef5e85285c4ac. These therefore make a good test case for these changes in core, as we don't want all plugins to start breaking.

@jsoref
Copy link
Contributor Author

jsoref commented Feb 12, 2019

Oh, I can write up a JEP for this if requested....

@jvz
Copy link
Member

jvz commented Feb 12, 2019

I'm not so sure about what you did moving those field attributes down from f:entry into child elements. That should be supported by using <f:prepareDatabing/> to set up field from the parent tag.

@oleg-nenashev
Copy link
Member

CC @recena who was voting for it a while ago

@daniel-beck daniel-beck added the web-ui The PR includes WebUI changes which may need special expertise label Feb 13, 2019
@daniel-beck
Copy link
Member

Impact on plugins is unclear. I fear if we expect all f:entry/f:whatever pairs to be updates, we won't see reasonable benefits here for a long, long time 😦

@jsoref jsoref force-pushed the JENKINS-56109 branch 2 times, most recently from a3e3134 to eef5612 Compare February 13, 2019 16:01
@jsoref
Copy link
Contributor Author

jsoref commented Feb 13, 2019

@jvz: oops, hazard of iterating between various attempts. Thanks, I've undone that.

@jsoref
Copy link
Contributor Author

jsoref commented Feb 13, 2019

Oops. No. The reason I moved the field was because I wanted to move the help. I'm going to put this back. Otherwise the help is floating aimlessly above the checkbox and looks ugly.

@jsoref
Copy link
Contributor Author

jsoref commented Feb 14, 2019

I'm going to make a couple of changes from what I have here:

  1. Adding group boundaries
  2. I think I'm going to move help to above things -- right now (this picture is from unpatched jenkins), the help revealer for a select dropdown can be virtually offscreen:
    image
    It either needs to be right below its item, or right above it, not below an entire super-expanding-group.

@jsoref

This comment has been minimized.

…s bad

This isn't the right fix, but someone needs to identify the proper content model
@daniel-beck
Copy link
Member

I provided @jsoref with https://gist.github.com/daniel-beck/ef4a6f38d78014343a0ef5e85285c4ac, a list of Jelly files in plugins that contain <td but not <table, indicating a reliance on existing structures at least in config.jelly and global.jelly files. These therefore make a good test case for these changes in core, as we don't want all plugins to start breaking.

@daniel-beck daniel-beck added the work-in-progress The PR is under active development, not ready to the final review label Feb 15, 2019
@jglick jglick added the unresolved-merge-conflict There is a merge conflict with the target branch. label Aug 31, 2020
@varyvol
Copy link

varyvol commented Sep 17, 2020

@jsoref could you please resolve the merge conflict?

@timja @daniel-beck @fqueiruga how do you see the current state for this change? Can it be moved forward?

@timja
Copy link
Member

timja commented Sep 17, 2020

@varyvol

I've been waiting for releases from the above people.

matrix-auth is now sorted thanks DB

reminder, could these be released please:

jenkinsci/promoted-builds-plugin#149 <- merged unreleased <- @oleg-nenashev

scm-api now released

@timja timja removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Sep 25, 2020
@timja
Copy link
Member

timja commented Sep 25, 2020

scm-api now released, pending any blockers we are probably good to move forward on this, probably discuss at next ux-sig meeting? (although I'm away for it)

@batmat
Copy link
Member

batmat commented Sep 29, 2020

First thanks a lot Tim for this stellar work, it's really an important update to Jenkins.

While there has unquestionably been awesome efforts to test and fix things in advance, I am inclined to recommend we merge this PR just after the next LTS baseline selection. This way, we would get the maximum time of weekly battle-testing for this change before it can reach the next-next LTS baseline.

Concretely: given the next baseline selection is planned for Wed 21st of October, I would propose we prepare this PR so it's ready-for-merge and on-hold before the meeting.
Then on this 21st of October, we merge this.

WDYT?

@daniel-beck
Copy link
Member

daniel-beck commented Sep 29, 2020

Agree with @batmat. TBH I'm really surprised that fairly minor PRs held this up for almost two months. Had I known these were the blocker, and not the 50 or so open issues in the Jira filter, I'd have made the time many weeks earlier to give this some more soaking time in weeklies.

@fqueiruga
Copy link
Contributor

+1 to what @batmat said.

@oleg-nenashev
Copy link
Member

Concretely: given the next baseline selection is planned for Wed 21st of October, I would propose we prepare this PR so it's ready-for-merge and on-hold before the meeting. Then on this 21st of October, we merge this.

Works for me. I would not mind if we do not wait until Oct 21 either. This story is pretty solid at the moment.

jenkinsci/promoted-builds-plugin#149 <- merged unreleased <- @oleg-nenashev

Releasing it now

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.

Released https://github.com/jenkinsci/promoted-builds-plugin/releases/tag/promoted-builds-3.6 . The code looks good to me. I have tested the last version with the Jenkins PR Tester, and I have not seen any issues. I am +1 for getting it released in the coming weeks, subject to a final discussion at the UX SIG.

Copy link
Member

@romenrg romenrg left a comment

Choose a reason for hiding this comment

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

This is a great step to modernize the Jenkins UI from the technical standpoint. Big improvement. Thanks to everyone involved, specially to @jsoref and @timja ! ❤️

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Oct 12, 2020

Do we have a consensus about merging the change. We have the LTS baseline selection scheduled to the next week. IIUC it is missing the next LTS baseline

@timja
Copy link
Member

timja commented Oct 12, 2020

UX sig meeting on wednesday

@timja
Copy link
Member

timja commented Oct 15, 2020

Discussed at the UX sig meeting:

Agreed we will ship it in the next weekly after baseline selection completed, (most likely 2.263).

🎉 thanks everyone for all your reviews and testing

@batmat
Copy link
Member

batmat commented Oct 22, 2020

Seems like it's time to un-on-hold this one and merge?

@timja timja added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed on-hold This pull request depends on another event/release, and it cannot be merged right now labels Oct 22, 2020
@timja
Copy link
Member

timja commented Oct 22, 2020

Seems like it's time to un-on-hold this one and merge?
🎉

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

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-rfe For changelog: Major enhancement. Will be highlighted on the top ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback squash-merge-me Unclean or useless commit history, should be merged only with squash-merge 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.