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

Refer to icons using classes instead of filenames #248

Merged
merged 3 commits into from
Jan 7, 2022

Conversation

zbynek
Copy link
Contributor

@zbynek zbynek commented Nov 12, 2021

SVG icons from core (added in #230 ) can be referenced by class instead of depending on file location in core. That should be more future-proof and seems to be the preferred way.

  • 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
  • [n/a] Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • [n/a] Ensure you have provided tests - that demonstrates feature works or fixes the issue

@zbynek
Copy link
Contributor Author

zbynek commented Nov 12, 2021

CC @NotMyFault @Wadeck @jglick

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 good to me, but it works as well on a lower baseline such as 2.277, based on my testing.

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.

Other than the BOM mistake, seems OK in principle, assuming it has been tested and indeed works as expected both on the newly lower baseline and on current weeklies. The icon class change seems right; the baseline version change seems unnecessary, especially given that we are already accommodating current plugin releases in jenkinsci/bom#691.

@@ -12,7 +12,6 @@
xmlns:inkscape="http://www.inkscape.org/namespaces/inkscape"
inkscape:export-ydpi="90"
inkscape:export-xdpi="90"
inkscape:export-filename="/Users/stephenc/src/plugins/cloudbees-registration-plugin/src/main/webapp/images/48x48/credentials.png"
Copy link
Member

Choose a reason for hiding this comment

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

Fine, but why here?

pom.xml Outdated Show resolved Hide resolved
@zbynek zbynek changed the title Make compatible with 2.289.1 LTS Make compatible with 2.303.1 LTS Nov 16, 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.

Looks OK. Not tested.

pom.xml Outdated Show resolved Hide resolved
@NotMyFault NotMyFault mentioned this pull request Nov 18, 2021
6 tasks
@zbynek zbynek changed the title Make compatible with 2.303.1 LTS Refer to icons using classes instead of filenames Jan 7, 2022
@jglick
Copy link
Member

jglick commented Jan 7, 2022

CI problem with Artifactory, trying to trigger a fresh build (for some reason the gesture on the Checks tab is not working)…

@jglick jglick closed this Jan 7, 2022
@jglick jglick reopened this Jan 7, 2022
@jglick jglick merged commit 60e6c29 into jenkinsci:master Jan 7, 2022
@zbynek zbynek deleted the svgs branch January 7, 2022 23:49
@timja
Copy link
Member

timja commented Jan 8, 2022

CI problem with Artifactory, trying to trigger a fresh build (for some reason the gesture on the Checks tab is not working)…

If checkout fails then blue ocean replay and checks re-run doesn't work. Needs a build now clicked or this

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.

4 participants