adambullmer / vue-cli-plugin-browser-extension

Browser extension development plugin for vue-cli 3.0
GNU Lesser General Public License v3.0
427 stars 76 forks source link

Add more options #14

Closed siphomateke closed 6 years ago

siphomateke commented 6 years ago
siphomateke commented 6 years ago

I am about to make some hard changes to the branch so hold of merging this for a little bit.

siphomateke commented 6 years ago

I'm done making changes so this is ready to be merged again.

adambullmer commented 6 years ago

Apologies for not responding sooner. Work has been a bit overbearing this week, and I've only now had the time to look at your submissions.

I disagree with making the icons optional, as they are an integral part of developing, and ultimately releasing an extension. Even if they are generic, boilerplate icons, I believe they should be included as is, and the consumer can just delete them if they decide to go against best practices. I want to steer away from this plugin becoming too configurable through the CLI/invocation as that leads to too much complexity in time. That being said I'd be interested to hear your reasoning for making them optional, as I might have missed why it would be important.

I think this is a lot of great work and would be more than happy to merge once we've had a bit of discussion around it. Thanks for your time!

adambullmer commented 6 years ago

Also looks like after merging #13 this PR needs to be rebased off master now

siphomateke commented 6 years ago

The reason I made them optional is because I wanted to put the icons in the public folder. Since vue-cli already copies all the files in the public folder to the output folder, rather than making the icon path configurable, I decided to allow disabling this plugin from handling icon copying.

I am fairly new to vue-cli so I may have missed the reasoning behind putting icons in the src folder. From what I understand, all assets that will never be processed by Webpack should go in the public folder. Perhaps I was wrong. If that is the case then I would recommend an icon path option so the developer can put the icons in the src/assets folder, for example.

I do agree with your reasoning for including the icons initially.

siphomateke commented 6 years ago

Rebased off master and fixed the docs due to the merge of #13

adambullmer commented 6 years ago

Ok, yeah, I think you're right about the purpose of location. I think the original reason it was there was because one of the libs I modeled this after did the same thing.

So we can instead, remove the optional part, scaffold the icons into the ./public/icons dir, and then remove the line that copies from the src lib (originally line 44 of the index.js). I'm all for less managed configuration, and as far as I can tell this solution meets all our needs. Great catch!

siphomateke commented 6 years ago

I've made the suggested changes.