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

[JENKINS-73156] handle svg cleanup of symbols via an xml document #9266

Merged
merged 1 commit into from
May 19, 2024

Conversation

mawinter69
Copy link
Contributor

@mawinter69 mawinter69 commented May 12, 2024

some attributes and elements where removed via regular expressions that removed more than what is necessary. Additionally stroke:#000 was replaced with currentColor because there are some icons that still use that.
This had side-effects when symbols are used that are not just black/white, e.g. using svgs from jenkins artwork via customizable-header plugin. There are constructs like using class attributes and a style definition referring to the classes. At other places there where color definitions with stroke:#000abc which were incorrectly modified.

svg cleanup is now implemented with DOM manipulation which is more robust.
5 icons that still used stroke:#000 are converted to use stroke:currentColor

opened jenkinsci/ionicons-api-plugin#64 as followup to fix some occurrences of stroke:#000

See JENKINS-73156.

Testing done

Manual testing, that the changed icons still work properly with dark theme.
Used some of the SVGs from https://www.jenkins.io/artwork/ and provided them as symbol in customizable-header plugin and verified they are properly shown

Proposed changelog entries

  • Handle svg cleanup via an xml document to avoid broken symbols.

Proposed upgrade guidelines

update ionicons-api plugin to at least version 74.v93d5eb_813d5f

Submitter checklist

Desired reviewers

@mention

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

Maintainer checklist

Document doc = db.parse(new StringInputStream(markup));
Element root = doc.getDocumentElement();
NodeList titleList = root.getElementsByTagName("title");
for (int i = 0; i < titleList.getLength(); i++) {
Copy link
Contributor Author

@mawinter69 mawinter69 May 12, 2024

Choose a reason for hiding this comment

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

As before just emptying the title elements. But why not just delete them completely?

some attributes and elements where removed via regular expressions.
Additionally `stroke:#000 was replaced with currentColor because there
are some icons that still use that.
This had sideeffects when symbols are used that are not just
black/white, e.g. using svg from jenkins artwork via customizable-header
plugin. There are construct like using class attributes and a style
definition referring to the classes. At other places there where color
definitions with `stroke:#000abc` which were incorrectly modified.

svg cleanup is now implemented with DOM manipulation which is more
robust.
5 icons that still used `stroke:#000` are converted to use
`stroke:currentColor`
@mawinter69 mawinter69 changed the title [JENKINS-73156] handle svg cleanup via an xml document [JENKINS-73156] handle svg cleanup of symbols via an xml document May 12, 2024
@timja timja added the bug For changelog: Minor bug. Will be listed after features label May 17, 2024
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.

/label ready-for-merge


This PR is now ready for merge. We will merge it after ~24 hours if there is no negative feedback.
Please see the merge process documentation for more information about the merge process.
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 May 18, 2024
@MarkEWaite MarkEWaite merged commit afaf96e into jenkinsci:master May 19, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For changelog: Minor bug. Will be listed after features ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants