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

chore: SVG optimization no longer breaks outline icons #1313

Merged
merged 2 commits into from
Dec 18, 2023

Conversation

liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Dec 14, 2023

Issue

Certain outline icons are being rendered as filled.

Why it's happening

The SVG optimization script currently causes some outline SVGs to be filled. SVGs with fill="none" set on the svg element has this attribute replaced by an ionicon-fill-none class:

item.class.add('ionicon-fill-none');

However, the optimized SVG template is then updated to a) remove all non-essential attributes from the svg element and b) add the ionicon class:

ionicons/scripts/build.ts

Lines 170 to 171 in 116d250

/<svg (.*?)>/,
`<svg xmlns="http://www.w3.org/2000/svg" class="ionicon" viewBox="0 0 512 512">`,

This causes the ionicon-fill-none class to be removed from the svg element. As a result, outline icons appear filled.

How I fixed it

This PR fixes the issue by removing the .replace call. During a previous optimization step I ensure that the ionicon class is added to the svg element. There are other attributes that need to be removed such as width and height. However, these are already being removed in

removeDimensions: true,
.

How to test this PR

  1. Clone the repo and check out icon-binocs. This is icon is being added in feat: add binoculars icons #1258 and reproduces this bug.
  2. Run npm run build.
  3. Open dist/svg/binoculars-outline.svg and verify that the icon appears filled.
  4. Merge that branch with this fix. git fetch && git merge origin/icon-svg-optimization.
  5. Run npm run build.
  6. Open dist/svg/binoculars-outline.svg and verify that the icon appears outlined.
Before After

@liamdebeasi liamdebeasi marked this pull request as ready for review December 14, 2023 20:37
@liamdebeasi liamdebeasi requested review from a team and sean-perkins and removed request for a team December 14, 2023 20:50
Copy link
Contributor

@sean-perkins sean-perkins left a comment

Choose a reason for hiding this comment

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

Verified against the reproduction steps, changes look good 👍

@liamdebeasi liamdebeasi enabled auto-merge (squash) December 18, 2023 20:25
@liamdebeasi liamdebeasi merged commit 64bbbf9 into main Dec 18, 2023
5 checks passed
@liamdebeasi liamdebeasi deleted the icon-svg-optimization branch December 18, 2023 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants