[JENKINS-75832] Support actions with Badges in the Hamburger Menu - #10791
Conversation
This adds support for Badges from RootActions in the Hamburger Menu (but more generically within dropdowns).
| </j:when> | ||
| </j:choose> | ||
| </j:set> | ||
| <l:overflowButton icon="symbol-menu-hamburger" id="header-more-actions" html="${hamburgerBadge}"> |
There was a problem hiding this comment.
passes the badge as HTML to the overflow button. whilst we could allow the overflow button to take a badge I felt that this was a bit specialized for an overflow button given that badges are for RootActions only (and management links), so took the generic approach of allowing arbitrary HTML.
| <j:set var="hamburgerBadge"> | ||
| <j:choose> | ||
| <j:when test="${hamburgerHasBadgeWithSeverityError}"> | ||
| <span class="jenkins-badge jenkins-!-danger-color" /> |
There was a problem hiding this comment.
this actually reveals a UX issue with the current design that is out of scope of this PR as it does exactly what is already done in the rest of the header.
Namely the visual indication of severity is only differentiated by colour. A colour blind user can not easily tell at a glance what the severity of the action is.
alecharp
left a comment
There was a problem hiding this comment.
I wonder if we could improve this later on by having the count of each severity and not just the higher severity.
This could help when users deliberately leave one warning but a second one appears.
However this is good to show badge in the hamburger menu.
I initially started with an approach like that, however the badge could get quite big "1 error, 3 warnings and 2 info", and showing this info in a tooltip would not solve the discoverability issue. There is certainly many UX improvements that could be made in the header, but doing it just in the hamburger but not addressing the Manage Jenkins cog (or others) would make it a bit disjointed. As such I think this is an improvement that is outside of this PR. |
|
From a UI perspective I would recommend to not differentiate between different badge types. So I think it would be simpler and more consistent if this is a boolean toggle: one of the containing elements has a badge or not. This also makes the UI much simpler: we just have one color (and icon) that shows that there is something important in the menu. This is the way others are doing it (see IntelliJ, GitHub, Element, MacOS, etc.): just one indicator that something needs your attention. When we use the suggested 3 types |
A great looking UI can have a really bad UX. I am hoping this was just an accidental miss-use of terms as we need to make the UX of Jenkins better not worse.
Define important? A badge can have an INFO so I would say calling out that something is broken and needs your attention at a glance is much better than calling out hey something has some extra info.. (We already do this on the Manage Jenkins cog). Additionally if there is an INFO you may choose to ignore it for a while, and then if an error comes along, you will not know something has become suddenly worse.
Jira (not a great UX, but neither is GH) )at least gives you a number and not just a single blob.
Its an defined enum in Jenkins core that has been in use for many years. It is already rendered differently in Jenkins core. Anyway - this discussion about future UX changes should not be happening in this ticket. The whole UX of Jenkins including the new header needs to be looked at holistically. I am not updating this PR to change the UI / badges as this is currently what Jenkins already does in the header for primary actions. Changing this is out of scope. |
This is exactly the same as the overlay as in the "manage Jenkins" cog (ref jelly), or any other primary actions with badges, I do not follow why the overlay badge here should be different? Either the overlay is globally bad, or it is globally OK. having different overlays meaning different things in different parts of the system would be confusing to users no? |
Personally at present I am of the belief it is globally bad as it is not accessible for colour blind users. However removing the status to me would be a step backwards and any change to badges in the header to align them all to something new (whatever that new is) is to me out of scope of this PR which fixes the badge where it is currently missing |
|
I have never seen those colored "point" badges without text in Jenkins before. But maybe that is because I am using Jenkins differently. If I had seen that earlier, I would have complained earlier (see the WCAG argument earlier by Judith). All badges I have seen in Jenkins (and other systems) have a number inside the badge or use a point overlay in a single color (without having a meaning for the color). |
| <h:secondaryAction action="${action}"/> | ||
| <j:set var="currentBadge" value="${action.badge}"/> | ||
| <j:if test="${currentBadge != null}"> | ||
| <j:choose> |
There was a problem hiding this comment.
untested but wouldn't this be a lot simpler:
diff --git i/core/src/main/resources/lib/layout/header/actions.jelly w/core/src/main/resources/lib/layout/header/actions.jelly
index 0bc9197cd7..0b42c6aba5 100644
--- i/core/src/main/resources/lib/layout/header/actions.jelly
+++ w/core/src/main/resources/lib/layout/header/actions.jelly
@@ -33,13 +33,13 @@
<j:if test="${currentBadge != null}">
<j:choose>
<j:when test="${currentBadge.severity == 'danger'}">
- <j:set var="hamburgerHasBadgeWithSeverityError" value="true" />
+ <j:set var="badgeColor" value="danger" />
</j:when>
<j:when test="${currentBadge.severity == 'warning'}">
- <j:set var="hamburgerHasBadgeWithSeverityWarning" value="true" />
+ <j:set var="badgeColor" value="warning" />
</j:when>
<j:otherwise>
- <j:set var="hamburgerHasBadgeWithSeverityInfo" value="true" />
+ <j:set var="badgeColor" value="info" />
</j:otherwise>
</j:choose>
</j:if>
@@ -48,17 +48,7 @@
</j:set>
<j:if test="${hamburgerEntries.length() gt 0}">
<j:set var="hamburgerBadge">
- <j:choose>
- <j:when test="${hamburgerHasBadgeWithSeverityError}">
- <span class="jenkins-badge jenkins-!-danger-color" />
- </j:when>
- <j:when test="${hamburgerHasBadgeWithSeverityWarning}">
- <span class="jenkins-badge jenkins-!-warning-color" />
- </j:when>
- <j:when test="${hamburgerHasBadgeWithSeverityInfo}">
- <span class="jenkins-badge jenkins-!-info-color" />
- </j:when>
- </j:choose>
+ <span class="jenkins-badge jenkins-!-${badgeColor}-color" />
</j:set>
<l:overflowButton icon="symbol-menu-hamburger" id="header-more-actions" html="${hamburgerBadge}">
<j:out value="${hamburgerEntries}" />There was a problem hiding this comment.
untested but wouldn't this be a lot simpler:
> - <j:set var="hamburgerHasBadgeWithSeverityError" value="true" />
> + <j:set var="badgeColor" value="danger" />
> </j:when>
> <j:when test="${currentBadge.severity == 'warning'}">
> - <j:set var="hamburgerHasBadgeWithSeverityWarning" value="true" />
> + <j:set var="badgeColor" value="warning" />
It would not work, as you would get the colour of the last badge seen from all the actions with badges, not the highest severity one.
e.g.
Action1 - no badge
Action2 - Badge:ERROR
Action3 - Badge:INFO
Action4 - no badge
when processing the list of actions
- (Action1 - no badge) - colour =
null - (Action2 - Badge:ERROR) colour =
danger - (Action3 - Badge:INFO) third action - colour =
info - (Action4 - no badge) colour =
info
In orther words this is to find the highest severity badge of all the badges to show in the ≡ button, not the individual actions that appear in the ≡
For whatever reason Badge takes a severity enum in the constructor, but the getSeverity returns a String. Otherwise yes it could be made simpler usign something like: severity = severity == null ? badge.getSeverityEnum() : severity.isHigherSeverity(badge.severity) ? severity : badge.severity (or the like)
But I did not want to introduce a new API at this point in time (esp as this needs to make the LTS), nor did I think doing String comparisons here would be a wise move.
Co-authored-by: Tim Jacomb <21194782+timja@users.noreply.github.com>
It was #10245 (the redesigned header) so unless you are using a weekly you would not have seen them. |
|
|
Please take a moment and address the merge conflicts of your pull request. Thanks! |
MarkEWaite
left a comment
There was a problem hiding this comment.
Works well in my interactive testing.
This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback.
/label ready-for-merge
This adds support for Badges from RootActions in the Hamburger Menu (but more generically within dropdowns). (cherry picked from commit 76eeaf1) From pull request: * jenkinsci#10791
(cherry picked from commit 0af57ba) Merge conflict resolved from commit 06d9b39 From pull request: * jenkinsci#10791
Co-authored-by: Tim Jacomb <21194782+timja@users.noreply.github.com> (cherry picked from commit 88d6a48) From pull request: * jenkinsci#10791




See JENKINS-75832.
Previously
RootActions that had a badge would not render the badge (correctly in the case of a anaction.jellyusingl:taskor at all when using the inbuilt rendering usingdd:item) when in a new Header and collapsed (either in the previous overflow or in the newer hamburger menu).This fixes the dom structure for
l:taskand adds the correct plumbing for dropdown menu items.Additionally to call out there is a badge in the hamburger the status is rolled up to the hambuger menu.
Testing done
Using a custom plugin with an action both with and without
action.jellywith and without a badge being present.Screenshots
Action with Badge and a tooltip hover.

Proposed changelog entries
Proposed changelog category
/label regression-fix
Proposed upgrade guidelines
N/A
Submitter checklist
@Restrictedor have@since TODOJavadocs, as appropriate.@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.evalto ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@mention
Before the changes are marked as
ready-for-merge:Maintainer checklist
upgrade-guide-neededlabel is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidateto be considered (see query).