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

add feature: svgo auto optimization to pull requests #1554

Merged
merged 7 commits into from
Feb 26, 2023

Conversation

lunatic-fox
Copy link
Contributor

@lunatic-fox lunatic-fox commented Nov 23, 2022

Double check these details before you open a PR

  • PR does not match another non-stale PR currently opened

Features

In optimize_icons.yml

This workflow runs only on pull requests using SVGO action and git-auto-commit Action.

In svgo.config.js

Converts style to inline attributes and removes dimensions, raster images and possible script elements.

In removeAttrs we can add some unnecessary atributes to be removed. Currently it will remove:

  • Empty fill
  • stroke and stroke derived attributes like stroke-width
  • Attributes that begin with data
  • enable-background
  • Some that we should handle with care
    • xml:space
    • fill-rule
    • clip-rule

This PR closes #1547, #1549 and partially #1548

Notes

Copy link
Collaborator

@Snailedlt Snailedlt left a comment

Choose a reason for hiding this comment

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

Looking forward to this feature, but it's not quite ready yet

.github/scripts/svgo.config.js Outdated Show resolved Hide resolved
.github/scripts/svgo.config.js Outdated Show resolved Hide resolved
.github/scripts/svgo.config.js Outdated Show resolved Hide resolved
@Snailedlt Snailedlt added enhancement devops Use this label for devops related enhancements labels Nov 28, 2022
@Snailedlt Snailedlt self-requested a review December 12, 2022 07:30
Snailedlt
Snailedlt previously approved these changes Jan 9, 2023
Copy link
Collaborator

@Snailedlt Snailedlt 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 as a starting point now. If this has been tested after the latest commit I'm okey with merging it in.

Good job @lunatic-fox ✔️

@Snailedlt
Copy link
Collaborator

@lunatic-fox This needs to be merged to master for the script to work right?

@lunatic-fox
Copy link
Contributor Author

@lunatic-fox This needs to be merged to master for the script to work right?

Actually no, it will work as soon as implemented like #1405. 😀

For instance, when the contributor makes a PR, the new branch will be based on develop branch. And this new branch going to be pull requested to develop. In summary, both branches have the workflow, the only exception would happen if the contributor base the PR on an old version of develop branch.

@Snailedlt
Copy link
Collaborator

But doesn't the workflow get the config file from the default (master) branch? At least that's the default behaviour for executing scripts from a workflow file iirc

@Panquesito7 Panquesito7 linked an issue Feb 8, 2023 that may be closed by this pull request
1 task
@lunatic-fox
Copy link
Contributor Author

As far as I tested, this new feature will work when merged to develop.

However, I found some issues when testing.

  • f2175f3 - Fix the bot permission to write and make a new commit.
  • ❌ We still need to create a short simple script to add a new line on each optimized SVG file, since SVGO does not do that. This step needs to occur after ericcornelissen/svgo-action@v3 step.
  • ⚠ Maybe, in order to be more selective, we could create a script to look if the file has more than 2 lines, if so activate this action, otherwise skip it. 🤔

@Panquesito7
Copy link
Member

We still need to create a short simple script to add a new line on each optimized SVG file, since SVGO does not do that. This step needs to occur after ericcornelissen/svgo-action@v3 step.

See #1624 for more information about this.

@lunatic-fox
Copy link
Contributor Author

But doesn't the workflow get the config file from the default (master) branch? At least that's the default behaviour for executing scripts from a workflow file iirc

As far as I got, the workflow files come from the branch that you're making the PR, that's why the #1332 works even when this feature is not implemented on master yet, because contributors are making PRs from develop that has this workflow file.

The issue #1411 is some exception and it's more related to templates, not to workflows. 😅

Copy link
Member

@Panquesito7 Panquesito7 left a comment

Choose a reason for hiding this comment

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

Otherwise, this is looking great. Thanks! 🚀

.github/scripts/svgo.config.js Show resolved Hide resolved
@Panquesito7 Panquesito7 merged commit 2160753 into devicons:develop Feb 26, 2023
@lunatic-fox lunatic-fox mentioned this pull request Mar 4, 2023
1 task
@lunatic-fox lunatic-fox deleted the bot_svgo branch June 1, 2023 12:29
@Snailedlt Snailedlt mentioned this pull request Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops Use this label for devops related enhancements enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST]: Automatically optimize icons with SVGO
3 participants