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

Remove unused material-icons #8831

Merged
merged 2 commits into from
Jan 13, 2024

Conversation

NotMyFault
Copy link
Member

@NotMyFault NotMyFault commented Jan 4, 2024

The change proposed removes the unused material-icons in favor of our standard ionicons used. There is no plugin with functional code left relying on the material-icons: https://github.com/search?q=org%3Ajenkinsci+NOT+repo%3Ajenkinsci%2Fjenkins+%22%2Fmaterial-icons%2F%22&type=code

Testing done

Jenkins and plugins function as-is, given the icons are unused.

Proposed changelog entries

  • Remove unused material icons.

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

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

Maintainer checklist

@NotMyFault NotMyFault added the skip-changelog Should not be shown in the changelog label Jan 4, 2024
@NotMyFault NotMyFault requested a review from a team January 4, 2024 21:59
timja
timja previously approved these changes Jan 4, 2024
@daniel-beck
Copy link
Member

daniel-beck commented Jan 4, 2024

in core

It's not in core though, it's a test? Doesn't this just reduce test coverage for something that still exists? Are there no plugins using these icons?

@NotMyFault
Copy link
Member Author

NotMyFault commented Jan 5, 2024

Are there no plugins using these icons?

The test case operates independently of the icon you reference. l:svgIcon is not reserved for material icons only, you can load any SVG you like, which is valid and intended.
The change proposed just changes which icon is used in the test, moving to our standard icon set, but that has no impact on coverage.

@NotMyFault NotMyFault changed the title Update the icon path in SvgIconTest Remove unused material-icons Jan 5, 2024
@NotMyFault NotMyFault added web-ui The PR includes WebUI changes which may need special expertise removed This PR removes a feature or a public API and removed skip-changelog Should not be shown in the changelog labels Jan 5, 2024
@NotMyFault NotMyFault requested a review from a team January 5, 2024 20:43
@daniel-beck daniel-beck dismissed timja’s stale review January 5, 2024 23:51

repurposed PR since review

@NotMyFault NotMyFault requested a review from timja January 6, 2024 22:18
@NotMyFault NotMyFault requested a review from a team January 6, 2024 22:22
@MarkEWaite
Copy link
Contributor

I see references to material-icons in the blue ocean plugin repository, but I'm not confident those are references to icons provided by core. Most of the references in that plugin seem to be to "blueocean-material-icons".

@NotMyFault
Copy link
Member Author

I see references to material-icons in the blue ocean plugin repository, but I'm not confident those are references to icons provided by core. Most of the references in that plugin seem to be to "blueocean-material-icons".

Indeed, they aren't shipped or referenced from core. BlueOcean has its own submodule containing the icons it needs.

At the time the material icons were added to core, no API was added to load them dynamically as you do it with symbols (<l:icon src="symbol-symbolName-outline plugin-ionicons-api" /> e.g.), but load them from a static path.
Plugins shipping their own icon set of material icons are unaffected by this change.

@NotMyFault NotMyFault requested review from a team and removed request for a team January 10, 2024 09:00
@NotMyFault NotMyFault merged commit faf22cd into jenkinsci:master Jan 13, 2024
17 checks passed
@NotMyFault NotMyFault deleted the update-svg-path-svgIcon branch January 13, 2024 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
removed This PR removes a feature or a public API web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants