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

feat: [JENKINS-66699] Exchange default PNGs with SVG icons #230

Merged
merged 3 commits into from
Oct 1, 2021

Conversation

NotMyFault
Copy link
Member

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

This PR aims to replace the PNGs with SVGs to present a more modern looking-like appearance.
Addresses https://issues.jenkins.io/browse/JENKINS-66699

Before:



After:



@NotMyFault NotMyFault changed the title Exchange default PNGs with SVG icons [JENKINS-66639] Exchange default PNGs with SVG icons Sep 22, 2021
@NotMyFault NotMyFault force-pushed the feature/master/exchange-icons branch 2 times, most recently from 8ec9ea9 to 2e2d03f Compare September 23, 2021 14:23
@jglick jglick changed the title [JENKINS-66639] Exchange default PNGs with SVG icons [JENKINS-66699] Exchange default PNGs with SVG icons Sep 28, 2021
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

SVG renames seem good. AFAICT there should be no new SVGs in this PR, because we should be picking them up from core sooner or later.

I suppose src/main/webapp/images/make-inkscape.sh should not get bundled in the binary.

@timja @uhafner @oleg-nenashev as reviewers of jenkinsci/jenkins#5663

@timja
Copy link
Member

timja commented Sep 28, 2021

I suppose src/main/webapp/images/make-inkscape.sh should not get bundled in the binary.

I think we removed the script from jenkinsci/jenkins as part of the original PR, it can probably be removed from here too?

@NotMyFault NotMyFault changed the title [JENKINS-66699] Exchange default PNGs with SVG icons feat: [JENKINS-66699] Exchange default PNGs with SVG icons Sep 28, 2021
@NotMyFault
Copy link
Member Author

I suppose src/main/webapp/images/make-inkscape.sh should not get bundled in the binary.

I think we removed the script from jenkinsci/jenkins as part of the original PR, it can probably be removed from here too?

I've removed it for now and removed the added svgs in favor of increasing the minimum version to 2.308.

@timja
Copy link
Member

timja commented Sep 28, 2021

You need to update jenkins.version in pom.xml for that

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Diff looks good. Needs an outside sanity check. Not sure who the plugin maintainer here is.

@timja
Copy link
Member

timja commented Sep 28, 2021

generally @jvz, but also @daniel-beck and @Wadeck by the looks of it

@timja timja requested a review from a team September 28, 2021 15:48
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.

Looks good to me. Will need an upgrade guide entry in the Jenkins core

@oleg-nenashev
Copy link
Member

I'm not sure I am still a maintainer. IIRC @daniel-beck has cleaned up my name at some point. Otherwise can release it

@timja
Copy link
Member

timja commented Sep 29, 2021

Looks good to me. Will need an upgrade guide entry in the Jenkins core

Probably overkill? If users don't do this they will see what they had before. When they upgrade they will get sharper icons. (but it can be done)

Copy link
Contributor

@Wadeck Wadeck left a comment

Choose a reason for hiding this comment

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

Except the user credentials icon, the SVGs are better, especially at higher level of zoom.

Thanks for the PR @NotMyFault

Screenshots

100%, before

before_100

100%, after

after_100

200%, before

before_200

200%, after

after_200

@KalleOlaviNiemitalo
Copy link

Could you add the jenkins.version change to the Credentials 2.6.2 release notes? This change caused Credentials 2.6.2 to be unavailable for the latest Jenkins LTS 2.303.2 (2021-10-06), and it would be good to have the reason clearly visible.

@timja
Copy link
Member

timja commented Oct 25, 2021

I've added it to the release notes, cc @jglick

@@ -67,7 +67,7 @@
<properties>
<revision>2.6.2</revision>
<changelist>-SNAPSHOT</changelist>
<jenkins.version>2.222.4</jenkins.version>
<jenkins.version>2.308</jenkins.version>
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@jglick
Copy link
Member

jglick commented Oct 25, 2021

In general, any plugin release may begin to require a newer version of Jenkins core. You should not normally need to pay attention to that, because the update center will only offer versions compatible with the core you are actually running; and PRs like jenkinsci/warnings-ng-plugin#1083 should not be necessary if you simply import bom (each BOM line will include only plugins compatible with the corresponding core LTS line).

@daniel-beck
Copy link
Member

Likely caused JENKINS-68674.

@jglick
Copy link
Member

jglick commented Jun 3, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants