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

Removed duplicated svgs #608

Closed
wants to merge 7 commits into from
Closed

Conversation

Thomas-Boi
Copy link
Member

This removes the duplicates listed in #607.

Notes:

  • Spring, perl, and phoenix's plain versions were the same as their original but with the color stripped. The font versions also have the color stripped so we don't have to worry much about this.
  • Lua actually don't have any duplicates, this has been crossed out in the issue
  • Also centered python-original.svg
  • Added screenshots folder to .gitignore. This is probably created due to one of the build steps

@Thomas-Boi
Copy link
Member Author

Since python-original.svg was changed, this is also our first official test of the svg-optimizer bot. Let's see how it do.

@Thomas-Boi
Copy link
Member Author

So it works great as expected. I'm also pleasantly surprised that the ids are prefixed properly when it's already prefixed.

@Thomas-Boi
Copy link
Member Author

Just updated the font files and the icomoon.json to the proper names. Also tested it and everything looks fine in the Icomoon website.

@Thomas-Boi Thomas-Boi requested review from amacado and Panquesito7 May 15, 2021 22:44
@Thomas-Boi
Copy link
Member Author

Just pushed a commit optimizing the python-wordmark-original.svg as seen in #103

new_icons.png
screenshots
Copy link
Member

Choose a reason for hiding this comment

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

Missing an endline.

Suggested change
screenshots
screenshots

Copy link
Member

@amacado amacado left a comment

Choose a reason for hiding this comment

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

I support the idea of reducing the overall size by removing duplicates. But since this is a breaking change I would like to discuss if it's worth it. On one hand we have the reduced file size on the other hand it could break existing applications when someone (like me) is using the icons in an automated way. How much space do we save when removing those icons?

@Thomas-Boi
Copy link
Member Author

I support the idea of reducing the overall size by removing duplicates. But since this is a breaking change I would like to discuss if it's worth it. On one hand we have the reduced file size on the other hand it could break existing applications when someone (like me) is using the icons in an automated way. How much space do we save when removing those icons?

Not a lot actually. The files are between 1 kB to 4 kB so I'll use 2 kB per file. There are 12 files removed => 12 * 2 kB = 24 kB. The benefit is small I admit and I have no strong feeling about the PR.

@Thomas-Boi
Copy link
Member Author

@amacado I think you are right. The file savings are not a lot but the potential backwards compatibility issue might be worth more. I'm cool if you want to close this PR. I'll make another one for the other fixes.

@amacado amacado closed this May 23, 2021
@Thomas-Boi Thomas-Boi deleted the thomas/feature/removeDuplicate branch May 24, 2021 19:03
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.

3 participants