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

Distinguish between new/aborted/disabled by ball color #3997

Merged
merged 1 commit into from May 7, 2019

Conversation

daniel-beck
Copy link
Member

@daniel-beck daniel-beck commented Apr 21, 2019

Implement suggestion from #3320 (comment). Minimally tested.

Try this PR: docker run --rm -ti -p 8080:8080 -e ID=3997 jenkins/core-pr-tester

Legend

Screen Shot

List view

Screen Shot

Proposed changelog entries

  • Allow visually distinguishing between new projects, disabled projects, and those with aborted builds through differently shaded build balls.

Submitter checklist

  • [n/a] 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.)
  • [n/a] Appropriate autotests or explanation to why this change has no tests
  • [n/a] For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@Ka-Wing

@jvz
Copy link
Member

jvz commented Apr 22, 2019

I suppose it's only slightly related, but you can likely just use SVG now as all web browsers support it. No need for rasterized copies.

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.

I would rather prefer something like #3320 (comment) instead of just adding more colors, but it is definitely a step forward. At least the balls now can be customized in Simple Theme plugin or in plugins like Green Balls.

@oleg-nenashev oleg-nenashev added the web-ui The PR includes WebUI changes which may need special expertise label Apr 26, 2019
@daniel-beck
Copy link
Member Author

At least the balls now can be customized in Simple Theme plugin or in plugins like Green Balls.

They always could…? I am not introducing new files. Just the legend screen was weird.

Copy link
Contributor

@fcojfernandez fcojfernandez left a comment

Choose a reason for hiding this comment

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

Thanks @dbekel ! Really useful PR IMO

@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Apr 26, 2019
@oleg-nenashev oleg-nenashev self-assigned this Apr 26, 2019
@@ -40,6 +40,8 @@
public static final Color YELLOW = new Color(0xFC,0xE9,0x4F);
public static final Color BLUE = new Color(0x72,0x9F,0xCF);
public static final Color GREY = new Color(0xAB,0xAB,0xAB);
public static final Color DARK_GREY = new Color(0x77,0x77,0x77);
public static final Color LIGHT_GREY = new Color(0xcc,0xcc,0xcc);
Copy link
Member

Choose a reason for hiding this comment

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

Oh, so it's this file that causes weird GUIs to appear in macOS.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jvz WDYM?

Copy link
Member

Choose a reason for hiding this comment

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

There seems to be a bug on macOS where referencing any AWT classes in a headless application will spawn a minimal GUI environment which is pretty much just an icon in the dock along with an application menu at the top. It steals focus from you while running the tests because macOS has yet to discover the joy of not allowing focus stealing. >:(

Copy link
Member

@jvz jvz 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 not super familiar with the features of SVG, but is it possible to use a single SVG for the balls and use CSS to choose the color?

@daniel-beck
Copy link
Member Author

I'm not super familiar with the features of SVG, but is it possible to use a single SVG for the balls and use CSS to choose the color?

I'm not going to figure out how to translate the animated icons into SVG for this, and without those, there's barely any benefit to that.

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

jvz commented Apr 26, 2019

I'm not super familiar with the features of SVG, but is it possible to use a single SVG for the balls and use CSS to choose the color?

I'm not going to figure out how to translate the animated icons into SVG for this, and without those, there's barely any benefit to that.

@daniel-beck
Copy link
Member Author

@jvz Feel free to amend this or propose a replacement PR.

@zbynek
Copy link
Contributor

zbynek commented Apr 26, 2019

Animation can be done for raster images or SVGs on client side, quick demo
https://codepen.io/anon/pen/OGdxew

Coloring from CSS works when you inline the SVG but (AFAIK) not for <img src=blue.svg>. So it should be doable to have one SVG per color and kill the animated gifs, but

use a single SVG for the balls and use CSS to choose the color

sounds tricky.

@oleg-nenashev
Copy link
Member

Maybe we could integrate it as is and create a follow-up ticket for enhancement which could be done by experts

@daniel-beck
Copy link
Member Author

Yup.

Would like to mention this is a strict improvement over today even if the orbs are ugly, since the legend is now properly split up.

@daniel-beck daniel-beck 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 May 5, 2019
@daniel-beck
Copy link
Member Author

Un-on-holding, as that was to get clarification about the Mac UI comment; it's unrelated to this PR.

@oleg-nenashev
Copy link
Member

Should we land it into the weekly since it gets delayed?

@daniel-beck
Copy link
Member Author

It's out now, so feel free to merge towards the next.

@oleg-nenashev
Copy link
Member

OK, let's integrate it so that we start building a new weekly. Will merge it tomorrow if there is no negative feedback

@oleg-nenashev oleg-nenashev merged commit f115ae0 into jenkinsci:master May 7, 2019
@daniel-beck
Copy link
Member Author

@stefandrissen This is not where you report bugs, please use our issue tracker for that.

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 web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
5 participants