Skip to content
This repository has been archived by the owner on Mar 17, 2023. It is now read-only.

Added ability to use additional optimizers #166

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

scorgn
Copy link

@scorgn scorgn commented Sep 5, 2018

To add the ability to use additional optimizers without editing the core loader, I have added the option additionalPlugins to the loader options. Using this, a user can define an additional imagemin plugin to use, along with it's options, and the loader will utilize that optimizer as well (as long as it is installed as a node module).

Shawn Corrigan added 3 commits September 4, 2018 21:07
To add the ability to use additional optimizers without editing the core loader, I have added the option additionalPlugins to the loader options. Using this, a user can define an additional imagemin plugin to use, along with it's options, and the loader will utilize that optimizer as well (as long as it is installed as a node module).
@tcoopman
Copy link
Owner

sorry for the late review. Can you add a test where you add an additional optimizer?

@anikethsaha
Copy link
Contributor

I think this is good.
Yeah, tests are required.
@tcoopman I guess you can add the tests in this PR.
or else I will fork @ShawnCorrigan 's branch and will add tests and submit a follow-up PR for the tests

@tcoopman
Copy link
Owner

tcoopman commented Dec 9, 2019

A follow-up PR with tests would be ok.

@anikethsaha
Copy link
Contributor

aah...I cant fork his repo and keeping my existing fork.
And even if I am forking his repo, I guess he is not active in GitHub so it might not get merge there.

I guess you can merge this and I will add tests

@scorgn
Copy link
Author

scorgn commented Dec 10, 2019 via email

@anikethsaha
Copy link
Contributor

I am active enough, I will accept any PRs towards my repo. I just never had time to write tests for this.

Ohh...I saw your GH contribution graph and thought of that. Nevermind. Will submit a PR in your fork soon

@scorgn
Copy link
Author

scorgn commented Dec 10, 2019

Sounds good! I'll keep an eye out for a PR. I am only contributing to private repos right now, but I do want to get back into open source development.

@anikethsaha
Copy link
Contributor

Sounds good! I'll keep an eye out for a PR. I am only contributing to private repos right now, but I do want to get back into open source development.

Great to hear that 👍

chore(test): added tests for additional plugin feat
@tcoopman
Copy link
Owner

Can the conflicts be resolved?

@anikethsaha
Copy link
Contributor

@ShawnCorrigan after rebasing , you need to update the schema.json as well to add this new configuration property.

@scorgn
Copy link
Author

scorgn commented Dec 13, 2019

Okay I will get to this tonight after work

@anikethsaha
Copy link
Contributor

Cc @ShawnCorrigan

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants