-
-
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
Replace static icon references with symbol API #7052
Conversation
I have no idea how to review this change. If you want me to approve it, you just need to convince me that you've tested the change sufficiently. Your PR has no "Testing Done" section, so while I generally trust your work I have no way of verifying that. |
No worries, I updated the PR description to include a bit more context, but I wouldn't mind if you pass on your review to other UX SIG members, if you prefer that. |
Before / After screenshots of every component changed here would make this easier rather than just the result |
There we go 👍🏻 |
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.
Tested locally, looks good and thanks for the screenshots
@NotMyFault I think this is worth adding to the changelog as a minor item, i.e. the user profile icon is changing |
This PR is now ready for merge. We will merge it after ~24 hours if there is no negative feedback. |
|
Discussed in PR, could go either way |
@@ -19,7 +19,7 @@ | |||
<j:if test="${attrs.iconSize != null}"> | |||
<j:set var="iconSize" value="icon-${iconSize}"/> | |||
</j:if> | |||
<l:svgIcon tooltip="${tooltip}" | |||
<l:icon tooltip="${tooltip}" |
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 SECURITY-2886.
This PR replaces a few static icon references with already used symbols through the symbol API. I plan to create a follow-up PR to address the leftover ones by adding the corresponding symbols.
noJob.groovy - the arrow shape change is marginal
Before
After
headerContent.jelly - Same symbol like on the profile page header
Before
After
<l:helpIcon>
Before
After
https://github.com/jenkinsci/jenkins/blob/master/war/src/main/resources/images/symbols/help-circle.svg
Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Proposed changelog entries
section only if there are breaking changes or other changes which may require extra steps from users during the upgrade@Restricted
or have@since TODO
Javadoc, as appropriate.@Deprecated(since = "TODO")
or@Deprecated(forRemoval = true, since = "TODO")
if applicable.eval
to ease future introduction of Content-Security-Policy directives (see documentation on jenkins.io).Desired reviewers
@mention
Maintainer checklist
Before the changes are marked as
ready-for-merge
:Proposed changelog entries
are accurate, human-readable, and in the imperative moodupgrade-guide-needed
label is set and there is aProposed upgrade guidelines
section in the PR title. (example)lts-candidate
to be considered (see query).