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

Send path to svgo #61

Merged
merged 5 commits into from Sep 1, 2022
Merged

Send path to svgo #61

merged 5 commits into from Sep 1, 2022

Conversation

mrleblanc101
Copy link

@mrleblanc101 mrleblanc101 commented Jul 7, 2022

Fix #60

@mrleblanc101 mrleblanc101 marked this pull request as ready for review July 7, 2022 21:33
@mrleblanc101
Copy link
Author

Tested with npm link and work great 👍

@jpkleemans
Copy link
Owner

Hi Sébastien, thanks for your PR! I'll look into it next week.

@mrleblanc101
Copy link
Author

Thanks

Copy link
Owner

@jpkleemans jpkleemans left a comment

Choose a reason for hiding this comment

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

Thanks for your work. Could you review my comments?
Also, is it possible for you to add a cypress test for this change?

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@mrleblanc101
Copy link
Author

mrleblanc101 commented Jul 15, 2022

Actually I think path should be passed to svgo.optimize({ path: 'path.svg' }) alone and the options should be passed to new svgo(options) but you don't seem to instantiate the plugin like that.

@mrleblanc101
Copy link
Author

mrleblanc101 commented Jul 15, 2022

Sébastien LeBlanc added 2 commits July 18, 2022 16:00
@mrleblanc101
Copy link
Author

Thanks for your work. Could you review my comments? Also, is it possible for you to add a cypress test for this change?

I added one test

@jpkleemans
Copy link
Owner

Actually I think path should be passed to svgo.optimize({ path: 'path.svg' }) alone and the options should be passed to new svgo(options) but you don't seem to instantiate the plugin like that.

In the svgo readme, it looks like you can add config to optimize just fine: https://github.com/svg/svgo#optimize

@jpkleemans
Copy link
Owner

Thanks for adding the test!
Can you fix the merge conflicts? Then I can merge the PR

@mrleblanc101
Copy link
Author

@jpkleemans I fixed the merge conflicts.

@jpkleemans jpkleemans merged commit ce3e75e into jpkleemans:main Sep 1, 2022
@jpkleemans
Copy link
Owner

Thank you!

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.

svgo.optimize missing path option
2 participants