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

Modernize svgo config #2301

Merged
merged 3 commits into from
May 5, 2024
Merged

Modernize svgo config #2301

merged 3 commits into from
May 5, 2024

Conversation

Repiteo
Copy link
Contributor

@Repiteo Repiteo commented Apr 22, 2024

Using the svgo command gives a warning that extendDefaultPlugins is now deprecated, instead recommending the new format seen here. While doing this, I tested some other optional features that weren't yet used; after seeing that they had negligible to nonexistant impact, these options have been added: multipass, convertStyleToAttrs, and sortAttrs

This command should probably be getting more love, as virtually every single svg can be optimized. Maybe a pre-commit hook could be considered? I've heard good things about Husky, might look into that later 🤔

Copy link
Member

@PKief PKief left a comment

Choose a reason for hiding this comment

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

Thanks a lot! I agree with you, that this command should definitely be used more often. As already mentioned (here), I'm not a big fan of pre-commit hooks. I would rather prefer it if we could, for example, optimise all SVG files as part of the release action.

@Repiteo
Copy link
Contributor Author

Repiteo commented Apr 27, 2024

Haven't messed with workflow actions much, but I'm pretty sure it's now setup to do just that!

@PKief
Copy link
Member

PKief commented Apr 28, 2024

@Repiteo It looks very good, it was exactly what I initially thought. I'll test your changes soon, but I'm agreeing with you, it should work like that.

Copy link
Member

@PKief PKief left a comment

Choose a reason for hiding this comment

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

I've updated the release.yml file a bit. It's important to run "git add ..." to include the changed files in the version commit. In addition, I've moved the "preversion" script into the release.yml file because it looks more structured and more clean.

@PKief PKief merged commit ab623a1 into material-extensions:main May 5, 2024
2 checks passed
Copy link

github-actions bot commented May 5, 2024

Merge Successful

Thanks for your contribution! 🎉

The changes will be part of the upcoming update on the marketplace.

@Repiteo Repiteo deleted the svgo branch May 5, 2024 19:01
@lucas-labs
Copy link
Member

I've updated the release.yml file a bit. It's important to run "git add ..." to include the changed files in the version commit. In addition, I've moved the "preversion" script into the release.yml file because it looks more structured and more clean.

@PKief, I think this will generate a npm ERR! Git working directory not clean error. It happened to me in my fork and I discovered that npm version apparently requires that all files are added as part of the version or preversion script.

See npm/npm#8620 and npm version documentation

From the docs:

Run the preversion script. These scripts have access to the old version in package.json. A typical use would be running your full test suite before deploying. Any files you want added to the commit should be explicitly added using git add.

Apparently, trying to git add files before preversion or version (e.g. as part of the release action) will throw:

npm ERR! Git working directory not clean.

PKief added a commit that referenced this pull request May 6, 2024
* Modernize svgo config

* feat: update release yml

---------

Co-authored-by: Philipp Kief <philipp.kief@gmx.de>
@PKief
Copy link
Member

PKief commented May 6, 2024

I've updated the release.yml file a bit. It's important to run "git add ..." to include the changed files in the version commit. In addition, I've moved the "preversion" script into the release.yml file because it looks more structured and more clean.

@PKief, I think this will generate a npm ERR! Git working directory not clean error. It happened to me in my fork and I discovered that npm version apparently requires that all files are added as part of the version or preversion script.

See npm/npm#8620 and npm version documentation

From the docs:

Run the preversion script. These scripts have access to the old version in package.json. A typical use would be running your full test suite before deploying. Any files you want added to the commit should be explicitly added using git add.

Apparently, trying to git add files before preversion or version (e.g. as part of the release action) will throw:

npm ERR! Git working directory not clean.

Thank you for the hint. You're right and I forgot about it. I changed it, so that all scripts are executed via "preversion" npm script.

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