-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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
Use symbols for build status #8705
Use symbols for build status #8705
Conversation
I see it in your screenshot to, maybe not required to fix it's just a bit higher than before. |
Ah sorry I misunderstood - the icon position is fixed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
/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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to object. There are still several places where the old things are used. I even found a place where an old blue ball is used.
E.g. create a freestyle job that triggers another freestyle job. This lead to this:
Maybe that old blue ball is coming from https://plugins.jenkins.io/parameterized-trigger/
Places with old implementation:
lib/hudson/buildLink.jelly#55
hudon/model/AbstractBuild/index.jelly#79,#89
hudson/model/Run/statusIcon.jelly#34 (not sure if this is anywhere used
lib/hudson/jobLink.jelly#34
Seems the blue ball was recently fixed: jenkinsci/parameterized-trigger-plugin#357 |
Updated those - thanks for finding. |
/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! |
@janfaracik thanks very much for the improvement in the build status display! I like it very much. It appears that the improvement has exposed two unit tests in the JUnit plugin that were dependent on the previous layout. Would you be willing to look at those two tests to adapt them so that they will pass on both Jenkins 2.433 (before the change) and Jenkins 2.434 (after the change)? Detected in plugin bill of materials pull request: Raised as a JUnit plugin issue in: If you're not able to investigate it, please let me know and I'll see if I can find someone else to help. |
jenkinsci/junit-plugin#588 is the issue report for the JUnit plugin tests that depend on the icon based build status display. From jenkinsci/jenkins#8705 the Jenkins core pull request that switched from using icon based build status to symbol based build status. That change also fixes a layout issue in the Safari web browser.
jenkinsci/junit-plugin#588 is the issue report for the JUnit plugin tests that depend on the icon based build status display. From jenkinsci/jenkins#8705 the Jenkins core pull request that switched from using icon based build status to symbol based build status. That change also fixes a layout issue in the Safari web browser.
function generateSVGIcon(iconName, iconSizeClass) { | ||
const imagesURL = document.head.getAttribute("data-imagesurl"); | ||
|
||
const isInProgress = iconName.endsWith("anime"); | ||
let buildStatus = "never-built"; | ||
switch (iconName) { | ||
case "red": | ||
case "red-anime": | ||
buildStatus = "last-failed"; | ||
break; | ||
case "yellow": | ||
case "yellow-anime": | ||
buildStatus = "last-unstable"; | ||
break; | ||
case "blue": | ||
case "blue-anime": | ||
buildStatus = "last-successful"; | ||
break; | ||
case "grey": | ||
case "grey-anime": | ||
case "disabled": | ||
case "disabled-anime": | ||
buildStatus = "last-disabled"; | ||
break; | ||
case "aborted": | ||
case "aborted-anime": | ||
buildStatus = "last-aborted"; | ||
break; | ||
case "nobuilt": | ||
case "nobuilt-anime": | ||
buildStatus = "never-built"; | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caused JENKINS-72407
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is not here but in StatusColumn -> column.jelly
} | ||
function generateSVGIcon(iconName) { | ||
const icons = document.querySelector("#jenkins-build-status-icons"); | ||
iconName = iconName.replace("-anime", ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason why the anime svgs are excluded? Before they were animated when the build was running
https://github.com/jenkinsci/junit-plugin/releases/tag/1252.vfc2e5efa_294f is the JUnit plugin release that fixes the test. Special thanks to Tim Jacomb for fixing the test in jenkinsci/junit-plugin#591 jenkinsci/junit-plugin#588 is the issue report for the JUnit plugin tests that depend on the icon based build status display. jenkinsci/jenkins#8705 is the Jenkins core pull request that switched from using icon based build status to symbol based build status. That change also fixes a layout issue in the Safari web browser. This reverts commit cb2376e. Author: Mark Waite <mark.earl.waite@gmail.com> Date: Tue Nov 28 22:00:27 2023 -0700
…c2e5efa_294f in /bom-weekly (#2755) * Bump org.jenkins-ci.plugins:junit in /bom-weekly Bumps [org.jenkins-ci.plugins:junit](https://github.com/jenkinsci/junit-plugin) from 1240.vf9529b_881428 to 1252.vfc2e5efa_294f. - [Release notes](https://github.com/jenkinsci/junit-plugin/releases) - [Commits](https://github.com/jenkinsci/junit-plugin/commits) --- updated-dependencies: - dependency-name: org.jenkins-ci.plugins:junit dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> * Revert "Exclude AggregatedTestResultPublisherTest" https://github.com/jenkinsci/junit-plugin/releases/tag/1252.vfc2e5efa_294f is the JUnit plugin release that fixes the test. Special thanks to Tim Jacomb for fixing the test in jenkinsci/junit-plugin#591 jenkinsci/junit-plugin#588 is the issue report for the JUnit plugin tests that depend on the icon based build status display. jenkinsci/jenkins#8705 is the Jenkins core pull request that switched from using icon based build status to symbol based build status. That change also fixes a layout issue in the Safari web browser. This reverts commit cb2376e. Author: Mark Waite <mark.earl.waite@gmail.com> Date: Tue Nov 28 22:00:27 2023 -0700 --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Mark Waite <mark.earl.waite@gmail.com>
@@ -52,7 +52,7 @@ THE SOFTWARE. | |||
</j:when> | |||
<j:otherwise> | |||
<a href="${attrs.href ?: rootURL+'/'+r.url}" class="model-link"> | |||
<l:icon alt="${r.iconColor.description}" class="${r.buildStatusIconClassName} icon-sm" style="margin-left: 0; position: relative; top: -0.1rem;"/><span class="jenkins-icon-adjacent">${jobName_}#<!-- -->${number}</span></a> | |||
<l:icon alt="${r.iconColor.description}" src="symbol-status-${r.iconColor.iconName}" class="icon-sm" style="margin-left: 0; position: relative; top: -0.1rem;"/><span class="jenkins-icon-adjacent">${jobName_}#<!-- -->${number}</span></a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTR: I filed https://issues.jenkins.io/browse/JENKINS-72491 |
Caused JENKINS-72711 |
Caused JENKINS-72845. |
I made https://issues.jenkins.io/browse/JENKINS-73386 |
Caused JENKINS-74868. |
Rather late followup to #7208 in which weather icons were replaced with symbols. This PR does the same for build status icons and also updates the appearance of some of them (do give feedback on this!).
The current approach to build status icons is a little unwieldy and can break in some scenarios (see below images). Using symbols is consistent with near enough every other icon in core Jenkins and keeps things simple to update in the future.
Before
After
Before (in Safari)
After
Testing done
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Desired reviewers
@jenkinsci/sig-ux
Before the changes are marked as
ready-for-merge
:Maintainer checklist