-
Notifications
You must be signed in to change notification settings - Fork 1
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
base: master
Are you sure you want to change the base?
Add flexibility through new options #5
Conversation
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. |
c3e223d
to
6e8b1e6
Compare
@nass600 sure, you're welcome. |
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
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 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? |
@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. |
This changeset adds a lot more flexibility to icomoon-builder through new CLI options. Some key highlights include:
icomoon-builder export font-name icomoon.zip --css=public/css/ --fonts=public/fonts/
would only copy CSS and fonts.@font-face
declaration).--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:
from
column, and relative to the current directory in theto
column. This avoids a hugely large table while still giving the useful information..tmp/
which might be used by other build systems, as well as leaving the directory in place when the command fails.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.