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

Add flexibility through new options #5

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

Conversation

alejandroiglesias
Copy link

@alejandroiglesias alejandroiglesias commented May 5, 2018

This changeset adds a lot more flexibility to icomoon-builder through new CLI options. Some key highlights include:

  • It's not mandatory to specify all the files' paths from the Icomoon zip file anymore. Now you opt-in for copying CSS, SCSS, fonts, docs, and the JSON file separately. Example: icomoon-builder export font-name icomoon.zip --css=public/css/ --fonts=public/fonts/ would only copy CSS and fonts.
  • Allows overriding the CSS file name and the public font path (the path used in the CSS @font-face declaration).
  • Allows toggling CSS minification. Currently still defaults to true and you can use --no-minify to opt out, but I think it should actually be an opt-in feature since minifying is not the main purpose of this library (and a lot of projects have a build system in place that takes care of that).

Also added smaller changes that improve the usability of the tool:

  • The table of files to be copied now shows file names relative to the zip file contents in the from column, and relative to the current directory in the to column. This avoids a hugely large table while still giving the useful information.
  • Unzips to a temporary directory in the OS. This avoids overwriting .tmp/ which might be used by other build systems, as well as leaving the directory in place when the command fails.
  • Small fixes and improvements.

Warning: this is a major changeset which is not backwards compatible with the current CLI API, so a new major 2.0.0 release would be necessary according to semver.

@nass600
Copy link
Owner

nass600 commented May 13, 2018

Hi Alejandro.

I was on holidays, sorry for the late response and thanks for your PR. Let me have a look into it in the upcoming days.

@alejandroiglesias
Copy link
Author

@nass600 sure, you're welcome.

@nass600
Copy link
Owner

nass600 commented May 19, 2018

Hi Alejandro, I finally have a chance to look into your PR.

Nice piece of work BTW, when I was creating this library I tried to make it as flexible as I could but
I found a couple of problems along the way, some of them are in the following list along with general errors or scenario-like inconsistencies:

  1. Default command does not fallback to a directory.

      export fancy-icons ~/Downloads/fancy-icons.zip

    The command displays an empty table of file changes and finishes without any change or file generated.

    If the rest of paths (options) are empty this still has to function what means making assumptions on where those files will be stored. In my opinion this should fallback to './'

  2. Absolute paths are not rendered in the results table.

    I kinda like this part so the table is more readable but in the from column there is still one absolute path, the css file that will be minified (if not using the --no-minify option).

    Apart from this little bug, this library is meant to be used as standalone also as in not only as npm dependency in a project. In such case I think the user is missing info on the to column when you are using paths like ../../whatever/something and he has to double check if the files will be generated in the folder he really wants.

    I think it would make sense to keep absolute directory rendering on the to column to give more info and remove the absolute path on the left as it kinda useless considering the from folder will be removed.

  3. Writes extra fonts directory to be used in the demo files (Duplicated fonts)

    export line-icons ~/Downloads/line-icons.zip --css=test-export/css --docs=test-export/docs/demo --fonts=test-export/fonts --json=test-export/docs --preprocessor=test-export/scss

    Running this example you will have the fonts generated in:

    • test-export/docs/demo/fonts
    • test-export/fonts

    This looks like made on purpose but I don't see the scenario where we need to duplicate them.

  4. The option --css-name overrides the minified version.

    export line-icons ~/Downloads/line-icons.zip --css=test-export/css --docs=test-export/docs/demo --fonts=test-export/fonts --json=test-export/docs --preprocessor=test-export/scss --css-name=other-name

    This will generate the following file test-export/css/cool-name and such file is the minified version
    as the non minified one got overriden.

    Also the option name "--css-name" tells me I only need to input the name and not the file extension.

    Cannot think of an scenario in which you will want to generate a css file without the css extension attached to it or having a totally different extension.

  5. Not adding --fonts throws an exception

    export line-icons ~/Downloads/line-icons.zip --css=test-export/css --docs=test-export/docs/demo --json=test-export/docs --preprocessor=test-export/scss

    As this is an option it should still work without using it but the whole point of this library is around the generated font what leads me to several questions:

    1. Should this be mandatory? In that case it cannot be an option
    2. Does it make sense to generate the css and scss files without this option. It's obvious it doesn't as the paths won't work.
  6. Using -css-name without --css or --fonts-public without --fonts

    export line-icons ~/Downloads/line-icons.zip --docs=test-export/docs/demo --fonts=test- 
    export/fonts --json=test-export/docs --preprocessor=test-export/scss --css-name=new-css- 
    name
    export line-icons ~/Downloads/line-icons.zip --docs=test-export/docs/demo --json=test- 
    export/docs --preprocessor=test-export/scss --fonts-public=new-font-name

    Should this throw an exception? What leads me once again there are conditional options with this
    approach, what makes the library extremely complex.

  7. --fonts-public affects not only the css but the scss file also.

    export line-icons ~/Downloads/line-icons.zip --css=test-export/css --docs=test-export/docs/demo --fonts=test-export/fonts --json=test-export/docs --preprocessor=test-export/scss --fonts-public=new-font-path

    Kind of makes sense to me this option but again this looks like another conditional option that should be allowed only if using --css along with it. Plus this cannot affect the sass file _variables.

  8. --no-minify without --css option.

    Another example of conditional options. Should this throw an exception? In my opinion it should but then it won't be an option...

Summary

As a summary, I truly expend a lot time thinking about how to make this library flexible but some of the cases/bugs stated here came to my mind before making it in this way.

I see a lot of features in your PR that make sense to me and are a really great addition, others are generating a couple of bugs or unexpected situations or use cases.

This library was meant to be expanded to use also less and stylus so imagine how the problems would exponentially grow making it that flexible.

Of course, I tried to make it usable for everyone (not only for my needs) and if you came with this PR is because I wasn't covering some scenarios like the ones you probably use so, changes need to be made.

The real big problem of this command is that it does a lot as in more than one responsibility meaning is not even close to SOLID principles but I cannot think of a way to break it down to several commands to be in charge of each kind of file without still having dependencies on the rest of the file types.

What do you think?

@alejandroiglesias
Copy link
Author

@nass600 sorry for not checking back. The changes I made were to be able to implement icomoon-builder into our asset management workflow and it's working quite good. I believe that these changes allow for a lot of flexibility that any project can benefit from, thus the PR, but I do not have more time to keep working on this. I suggest we can keep this open to see if I have some spare time in the near future to address the issues you listed and make it move forward. Otherwise, you can take advantage of the features that seem straightforward and don't cause issues, and discard the others.

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.

None yet

2 participants