Refactor plugin #39
Refactor plugin #39
Conversation
There was a problem hiding this comment.
A couple relatively small things, but overall looks great. Thanks so much for taking point on this @Exelord!
src/index.js
Outdated
| iconPath = path.join(this.inputPaths[0], iconPath); | ||
|
|
||
| if (!fs.existsSync(iconPath)) { | ||
| return Promise.resolve() |
There was a problem hiding this comment.
Should we print out a warning of some kind here? I could imagine someone getting confused about where to put the source file (i.e. they drop in the wrong folder) and then getting frustrated by the fact that the addon appears to not be working. Since all of this is opt-in, I can't imagine a scenario where you'd legitimately want to have this addon installed, but not have the source icon file exist.
There was a problem hiding this comment.
Well, there is a case. Eg you want to test out PWA thanks to generated manifests and add the favicon later.
But sure. That would be appreciated to add a warning like: [WARRNING] Favicon file has been not detected in path "favcon/path/file.png"
I will add that!
|
|
||
| const config = { | ||
| onSuccess() {}, | ||
| iconPath: 'favicon.png', |
There was a problem hiding this comment.
Shouldn't this default to something like public/favicons.png? I don't think the source image belongs at the project root level.
There was a problem hiding this comment.
Not really. This project should be fully independent, and not aware of Ember structure. It's working on a passed input and it should look into the input for the icon directly, by default
There was a problem hiding this comment.
Ah right, ember-cli-favicon should default to that, good point 👍
|
@davewasmer approved? :) |
This PR will:
Also, this marked the plugin as 2.0 version as I changed api structure. From now on, we can distignish plugin options from favicons.
This is a first PR from improvements series :)