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

Add updates count badge to Updates sidebar item #7084

Merged
merged 26 commits into from
Apr 16, 2023

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented Sep 8, 2022

Screenshot 2022-09-08 at 21 21 57

Adds back the updates count badge introduced and removed in #6783, this time by adding an attribute to the task.jelly component so that any view/plugin can use it.

Proposed changelog entries

  • Add updates count badge to Updates sidebar item.

Proposed upgrade guidelines

N/A

Submitter checklist

  • (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change) and are in the imperative mood. Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
  • New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO") if applicable.
  • New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content-Security-Policy directives (see documentation on jenkins.io).
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@jenkinsci/sig-ux

Maintainer checklist

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

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@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 needs-security-review Awaiting review by a security team member labels Sep 8, 2022
@timja timja requested a review from a team September 8, 2022 21:18
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.

lgtm, tested locally

image

image

image

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.

Looks nice :)

(feedback is non-blocking)

core/src/main/resources/lib/layout/task.jelly Outdated Show resolved Hide resolved
Co-authored-by: Alexander Brandes <brandes.alexander@web.de>
@daniel-beck daniel-beck 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 Sep 9, 2022
Co-authored-by: Daniel Beck <1831569+daniel-beck@users.noreply.github.com>
daniel-beck
daniel-beck previously approved these changes Sep 9, 2022
Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

Looks reasonable. Implementation is fine.

There should be usage guidelines.

@timja
Copy link
Member

timja commented Sep 9, 2022

There should be usage guidelines.

@janfaracik is that something you're able to add to design library before we ship this?

@daniel-beck daniel-beck dismissed their stale review September 9, 2022 10:49

missed something

@janfaracik
Copy link
Contributor Author

There should be usage guidelines.

@janfaracik is that something you're able to add to design library before we ship this?

Will do, am I good to add it to the existing 'Layouts' PR?

@daniel-beck
Copy link
Member

daniel-beck commented Sep 9, 2022

I think there should there be support for badges in context menus. For a generalized feature (e.g. imagine badges with open issues in various warnings-ng contributed sidepanel tasks) it probably makes sense to support that. Another use case of an improved task API for the context menu would be Manage Jenkins and entries like "Manage Old Data" or "In-process Script Approval" that could show a badge.

@timja
Copy link
Member

timja commented Sep 9, 2022

Will do, am I good to add it to the existing 'Layouts' PR?

Yes 👍

@janfaracik
Copy link
Contributor Author

Will do, am I good to add it to the existing 'Layouts' PR?

Yes 👍

Updated that PR with some examples of when to use badges:

image

@timja
Copy link
Member

timja commented Sep 9, 2022

Do you plan to address Daniel's comment on badges in context menu's in this PR?

@janfaracik
Copy link
Contributor Author

Do you plan to address Daniel's comment on badges in context menu's in this PR?

I'd favour doing them separately to this PR (just to get this in as I think its a nice-to-have), happy to do it though in this if you prefer @daniel-beck?

@daniel-beck
Copy link
Member

I'd prefer that this is done at the same time, to have parity between sidepanel and menus. (Unless there's a UX reason that menus should not have these badges, now or later, of course.)

@timja
Copy link
Member

timja commented Oct 20, 2022

@janfaracik do you plan to take a look at the context menu changes?

@janfaracik
Copy link
Contributor Author

@janfaracik do you plan to take a look at the context menu changes?

I will do for sure, just a little swamped at the moment.

@github-actions
Copy link

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Mar 25, 2023
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.

The number in the plugin manager could use the same color as the badge in the manage view:
Screenshot 2023-03-25 at 12 58 08
But I consider this non-blocking feedback, thanks for your work!

@daniel-beck daniel-beck added security-approved @jenkinsci/core-security-review reviewed this PR for security issues needs-security-review Awaiting review by a security team member and removed needs-security-review Awaiting review by a security team member security-approved @jenkinsci/core-security-review reviewed this PR for security issues labels Mar 28, 2023
Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

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

The tooltip isn't just in a "plain" HTML environment, but in an HTML attribute.

A tooltip of " onmouseenter=alert(1) " causes XSS.

As a general recommendation, stop concatenating strings to create HTML entirely.

@daniel-beck daniel-beck 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 Apr 13, 2023
@janfaracik
Copy link
Contributor Author

Is this PR good for merge notice now @timja?

@timja
Copy link
Member

timja commented Apr 13, 2023

/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 Apr 13, 2023
@NotMyFault NotMyFault merged commit 291f5ed into jenkinsci:master Apr 16, 2023
@@ -75,6 +75,9 @@ THE SOFTWARE.
Generally used with post="true".
(onclick supersedes this except in the context menu.)
</st:attribute>
<st:attribute name="badge" since="TODO">
If set, displays the value as a small badge on the right side of the sidepanel item.
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a better description. At the moment most people will assume this is just a simple text you can pass in but what is actually is required is a jenkins.management.Badge object.

@janfaracik janfaracik deleted the add-updates-badge branch May 5, 2023 12:15
MarkEWaite added a commit to MarkEWaite/jenkins that referenced this pull request May 25, 2023
…em (jenkinsci#7084)"

The "Manage Jenkins" / "System" drop down menu includes the extra text
"undefined" at the end of each entry.  This removes that change and
removes the updates count badge from the updates sidebar.

This reverts commit 291f5ed.
MarkEWaite added a commit that referenced this pull request May 26, 2023
[JENKINS-71345] Revert "Add updates count badge to Updates sidebar item (#7084)"

The "Manage Jenkins" / "System" drop down menu includes the extra text
"undefined" at the end of each entry.  This removes that change and
removes the updates count badge from the updates sidebar.

This reverts commit 291f5ed.
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
6 participants