christian-reichart / svg-chameleon

Easily modify stroke-widths and colors of your SVGs via CSS!
18 stars 4 forks source link

load options from either js or json file #8

Closed harterSe closed 4 years ago

harterSe commented 4 years ago

Suggestion for using either chameleon.js or chameleon.json file for configuration. #6 Also added a test folder with basic example svgs Please check if this is a valid solution.

christian-reichart commented 4 years ago

@harterSe Thanks for the PR. The test directory itself is a good idea, however I see a problem with the html file, since you can't read the local sprite file in the browser (security issue). Does this work on your machine somehow? What I did in the past was a seperate vue project to test out my icons in the browser, since the page there is delivered by a local server and the icons are displayed correctly. I always install the local changes of my svg-chameleon code by running npm i local/path/to/svg-chameleon in my vue project to get the latest changes. I think if this issue can't be solved I wouldn't include test files directly in the repo, since you could only check the sprite code in the IDE but not really test if the icons look correct.

Will test the config stuff later.

christian-reichart commented 4 years ago

Regarding the config files:

I would rename them to chameleon.config.js / json one major issue I see is that the config file doesn't get merged with the default options. It should be possible to just set some options and let the rest be default. I do this with the applyCustomOptions() already.

Also I think it would be great to give a --config=path/to/config.js option, if you don't want or can't store the file in the root directory.

christian-reichart commented 4 years ago

@harterSe Also really like the join function, didn't know this exists, but this solves the path issue for windows. Just not working correctly when no path is given I think. it joins the full path 2 times there I think.

harterSe commented 4 years ago

Regarding the security issues. I served the html file with the vscode live server plugin: https://marketplace.visualstudio.com/items?itemName=ritwickdey.LiveServer Opening the html from context menu in webstorm worked for me too. Another Solution is to use npx serve in the test directory to start a local dev server.

I will remove the test directory from the PR for now because it's not directly related to the config files. I think if a example/test directory is a good feature it cloud be done in another PR.

harterSe commented 4 years ago

I will:

I would change the merge as follows if this is sufficient.

  1. check if --config is present otherwise search for config files
  2. merge cli options into config file options
  3. merge options from 2. into default options.
christian-reichart commented 4 years ago

@harterSe sounds good!

Will checkout this vscode plugin and create another issue regarding the tryout setup.

Edit: Just for clarification regarding the path stuff: If no path is given, cwd should be taken.

harterSe commented 4 years ago

@christian-reichart

Just to check: Should it be possible to use configuration files if the tool is used from within the code? Or should we only look for configuration files / --config options when using the cli.

I think only for the cli would be the right way. But I am not sure about that.

TheJotob commented 4 years ago

Yes. I'd say so aswell. If you call it from code you can set the proper configuration there or load the configuration file before and inject it in your code yourself. So +1 for only loading the config file for CLI-use :)

christian-reichart commented 4 years ago

@harterSe look good, thank you!