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-69113] renovate progressBar #8821

Merged
merged 12 commits into from
Feb 19, 2024

Conversation

mawinter69
Copy link
Contributor

@mawinter69 mawinter69 commented Jan 2, 2024

The progressbar used at various places is replaced by a div/span combination and gets a new styling that is in sync with other ui elements. It is a little bit higher, has rounded corners and some padding, the minwidth is set to 10% so we always have at least a bit to show, it also avoids that the border is not rounded, e.g. when we're at just 2% and the progressbar is only 2px wide.
Animation is now based on css and no longer a gif and can also be enabled via a flag.
The build progress bar shown on run and console views is now updated dynamically including the tooltip.
The progress bar used in progressive rendering is doubled in size to make it more prominent that it is still running (See [JENKINS-72138], this doesn't solve the problem but might lower the chance that people reload the mentioned page because they think nothing happens).
This also fixes JENKINS-68631

After
Build history widget:
image

Executor widget:
image

Run view:
Here the progress bar will change over time
image
image

Console view with stuck:
image

Progressive rendering (agent build history) with bigger progress bar:
image

See JENKINS-69113.

Testing done

Manual testing:

  • create job that runs for 1 minute
  • trigger job (no previous builds) -> progress bar is at 100% and animated (on executor widgets, run page and console page)
  • trigger job (with previous builds) -> progress bar is animated and increases over time

To see stuck jobs

  • Create job that does nothing
  • run the job several times
  • Change job to sleep for 1 minute
  • run job -> the progressbar should now be red animated.

Tested for freestyle and pipeline jobs

Proposed changelog entries

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

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

Maintainer checklist

The progressbar used at various places is replaced by a div/span
compbination and gets a new styling that is in sync with other ui
elements. Mainly it has rounded corners.
The bar is always animated opposed to old behaviour where it was only
animated when the estimated remaining time was unknown. Animation is now
based on css and no longer a gif.
All colors are defined via variables so they can be adjusted by themes.
The build progress bar shown on run and console views is now updated
dynamically.
The progress bar used in progressive rendering is doubled in size to
make it more prominent that it is still running (See [JENKINS-72138],
this doesn't solve the problem but might lower the chance that people
reload the mentioned page because they think nothing happens).
@mawinter69 mawinter69 marked this pull request as draft January 2, 2024 01:37
@timja timja requested a review from janfaracik January 2, 2024 08:14
@timja timja added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted web-ui The PR includes WebUI changes which may need special expertise labels Jan 2, 2024
@timja timja requested a review from a team January 2, 2024 08:15
the parameters page also includes the buildCaption.jelly. But the js is
using a relative url thus accessing `statusIcon` fails.
Store status url in the div and read from there.
@mawinter69 mawinter69 marked this pull request as ready for review January 2, 2024 11:19
@janfaracik
Copy link
Contributor

Looking good!

I was looking at doing something similar last year, but never finished it. Here’s the branch if it’s at all helpful - https://github.com/jenkinsci/jenkins/compare/master...janfaracik:jenkins:progress-bar-new?expand=1.

Screen.Recording.2024-01-03.at.13.51.48.mov

A few things -

  • I think it’d be best to use the existing colour palette for consistency rather than using the colours from the existing progress bar, e.g. using the accent colour for progress, error colour for errors etc.
  • I think there should be separate animations for progress/indeterminate states, e.g. a soft pulse for progress, horizontal translation when indeterminate
  • It’s a little chunky at the moment and draws a lot of attention - maybe that’s okay though?

Let me know your thoughts 👍

@NotMyFault
Copy link
Member

Looking good!

I was looking at doing something similar last year, but never finished it. Here’s the branch if it’s at all helpful - master...janfaracik:jenkins:progress-bar-new?expand=1 (compare).

Screen.Recording.2024-01-03.at.13.51.48.mov
A few things -

  • I think it’d be best to use the existing colour palette for consistency rather than using the colours from the existing progress bar, e.g. using the accent colour for progress, error colour for errors etc.
  • I think there should be separate animations for progress/indeterminate states, e.g. a soft pulse for progress, horizontal translation when indeterminate
  • It’s a little chunky at the moment and draws a lot of attention - maybe that’s okay though?

Let me know your thoughts 👍

I'm a big fan of your proposed implementation of the progress bars. I agree we should stick with the current color palette for consistency :)

use existing colors
only animate when unknown or with flag
animate via build caption and progressive rendering
@mawinter69
Copy link
Contributor Author

mawinter69 commented Jan 3, 2024

The animation is now only used in the widgets when the progress is unknown or is is set via an option.
Using existing color palette now
moved the css to separate file.

@mawinter69
Copy link
Contributor Author

Should the padding be a bit bigger maybe?
like this:
image

@daniel-beck daniel-beck added the needs-test-fix One or more test case(s) need to be updated label Jan 4, 2024
@timja timja removed the needs-test-fix One or more test case(s) need to be updated label Jan 4, 2024
@NotMyFault NotMyFault requested review from janfaracik and a team January 10, 2024 09:12
@NotMyFault NotMyFault added the needs-security-review Awaiting review by a security team member label Jan 10, 2024
@yaroslavafenkin yaroslavafenkin added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Jan 11, 2024
Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

I'm really fond of the redesign, thanks!

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

/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 Feb 18, 2024
@NotMyFault NotMyFault merged commit 5304da1 into jenkinsci:master Feb 19, 2024
17 checks passed
@basil
Copy link
Member

basil commented Feb 21, 2024

Breaks jenkins.branch.NoTriggerBranchPropertyTest#suppressNothing and jenkins.branch.NoTriggerBranchPropertyTest#suppressEvents:

java.lang.AssertionError: 

Expected: is <2>
     but: was <3>
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:20)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:6)
	at jenkins.branch.NoTriggerBranchPropertyTest.suppressNothing(NoTriggerBranchPropertyTest.java:135)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:656)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
	at java.base/java.lang.Thread.run(Thread.java:1583)

@mawinter69
Copy link
Contributor Author

I wonder how this change could cause this error. This change only affect jelly/js files so it is pure UI

@basil
Copy link
Member

basil commented Feb 21, 2024

Are you implying that this PR may not be the cause of the test failures? The tests deterministically fail with this PR and pass when this PR is reverted, so this PR is the (proximate) cause of the test failures.

@jglick
Copy link
Member

jglick commented Feb 23, 2024

I also got here via git bisect. NoTriggerBranchPropertyTest runs JenkinsRule.configRoundtrip so it does exercise GUI code.

This PR also causes a failure in the ATH BuildHistoryTest (both test cases) at https://github.com/jenkinsci/acceptance-test-harness/blob/0ac2b663128e2c20bfdcacbf095496bd7c6b43ab/src/main/java/org/jenkinsci/test/acceptance/po/BuildHistory.java#L31.

@mawinter69
Copy link
Contributor Author

It's definitely happening after configRoundTrip
But I think it is not related to this change.
When I add a 30 sec sleep after the configRoundTrip, then also the current master of branch-api plugin built against 2.426.1 fails.
Isn't there an automatic first run triggered after saving the configuration?

@jglick
Copy link
Member

jglick commented Feb 23, 2024

Thanks, I can take a look at whether NoTriggerBranchPropertyTest is just overly sensitive to timing.

krisstern pushed a commit to krisstern/jenkins that referenced this pull request Apr 2, 2024
* [JENKINS-69113] renovate progressBar

The progressbar used at various places is replaced by a div/span
compbination and gets a new styling that is in sync with other ui
elements. Mainly it has rounded corners.
The bar is always animated opposed to old behaviour where it was only
animated when the estimated remaining time was unknown. Animation is now
based on css and no longer a gif.
All colors are defined via variables so they can be adjusted by themes.
The build progress bar shown on run and console views is now updated
dynamically.
The progress bar used in progressive rendering is doubled in size to
make it more prominent that it is still running (See [JENKINS-72138],
this doesn't solve the problem but might lower the chance that people
reload the mentioned page because they think nothing happens).

* apply prettier

* scss style

* set status url

the parameters page also includes the buildCaption.jelly. But the js is
using a relative url thus accessing `statusIcon` fails.
Store status url in the div and read from there.

* apply behaviour so tooltip is shown after icon update

fix url

* incorporate feedback

use existing colors
only animate when unknown or with flag
animate via build caption and progressive rendering

* adjust class name

* adjust bg color

* fix style

* sRGB

* avoid j:out

---------

Co-authored-by: Alexander Brandes <[email protected]>
(cherry picked from commit 5304da1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted security-approved @jenkinsci/core-security-review reviewed this PR for security issues 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.

8 participants