ben-eb / gulp-svgmin

Minify SVG files with gulp.
MIT License
341 stars 35 forks source link

Initial changes to accept new preset default object setting in svgo #124

Closed djphan closed 2 years ago

djphan commented 2 years ago

Some initial changes to manage the extension of preset defaults. I updated the tests so they passed and tests some basic configs with importing a svgo.config.js file and setting a rule that was available in the presets.

I didn't test this with custom plugins but those tests passed? So might need a once over on that

djphan commented 2 years ago

Related to https://github.com/ben-eb/gulp-svgmin/issues/122

rejas commented 2 years ago

Thanks for the PR. Unfortunately the automatic tests dont get run at the moment since travis changed their policy. I created https://github.com/ben-eb/gulp-svgmin/pull/123 to get that fixed and switch to github actions for that. Lets hope @ben-eb can take a look at this and merge some stuff.

djphan commented 2 years ago

That's good to know. I did have to adjust the tests and ran them manually locally. But it'll be good to get that going.

rejas commented 2 years ago

My PR got merged, could you rebase yours against the maste rbranch so we can see if the tests work?

djphan commented 2 years ago

Ok I'll take a look today and update the branch

djphan commented 2 years ago

I merged in the latest master to this PR. If someone could help verify @rejas @ben-eb that'll be awesome :)

djphan commented 2 years ago

Hmm looks like node 16 is failing due to a dependency issue on node 16. I'm using nvm locally on node version v14.17.6. npm version 7.24.0. Let me know what I can do to regenerate the package.json to unblock the checks.

But it looks like the checks are working

djphan commented 2 years ago

Ah thanks for the catch. I derp'd with my work laptop settings but will regenerate the package-lock without them.

I'll try and get something in end of day today or tomorrow morning. Just commenting for other folks tracking this PR

djphan commented 2 years ago

Updated now!

djphan commented 2 years ago

I got some emails that the workflows were not ran. Might want to check on that and let me know if one of my changes bork'd something?

rejas commented 2 years ago

I think its more an issue on how the repo ist configured: https://github.community/t/checkout-v2-error-actions-in-this-workflow-must-within-a-repository-owned-by-organization/136315/2 Maybe @ben-eb can correct it?

ben-eb commented 2 years ago

@rejas @djphan I've updated the settings but I think you might need to push again to trigger the build? Thanks 👍

djphan commented 2 years ago

Pushed and 🤞

coveralls commented 2 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 6f9976f7ccd4d2fee017f3a9967718bb5ce7a8d1 on Dotdashcom:update-svgo-presets into 99614f595200c6952187347a583141633c0e12ba on ben-eb:master.

rejas commented 2 years ago

Looks good, shall we merge and release @ben-eb ?

ben-eb commented 2 years ago

@rejas Agreed! 🚀

@djphan Thanks for your hard work! 👍

rejas commented 2 years ago

Merged. The question I have now is if this PR requires a major version update. I guess not since svgo didnt update their major version either, but I just want to make sure I dont miss anything changing @djphan @ben-eb

djphan commented 2 years ago

Oh I just noticed that I didn't update the package.json to be 2.4.0+. It might be a good idea to update that version to pull from a later svgo explicitly (oops) before cutting a version. SVGO had it as a minor version change and folks who might need an older gulp-svgmin can hard pick a version for their projects IMO.

ben-eb commented 2 years ago

@rejas I think it's reasonable to make major versions of this plugin whenever SVGO releases a new major version, other than that I'm not worried about how many minor releases we want to make. 👍