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

feat: add template options #20

Merged
merged 9 commits into from Nov 13, 2020

Conversation

janbiasi
Copy link
Collaborator

@janbiasi janbiasi commented Nov 12, 2020

Take a look at #12 for all details

CLI

  • adding optional --css-template flag
  • adding optional --html-template flag
  • adding optional --sass-template flag
  • adding optional --scss-template flag

Module

  • adding an optional template object where you can define template paths for custom CSS, HTML, SASS and SCSS templates
  • adding tests for the SASS and SCSS generators
  • resolve templates from process.cwd() per default if the path is not absolute

SASS & SCSS templates

By default it will generate a variable for the font name ($<name>-font) as well as a map for the icons ($<name>-map) so the developers can pick their icon via map-get($example-map, 'my-icon')

@casaper
Copy link
Collaborator

casaper commented Nov 12, 2020

oh, @janbiasi seems that we both did the same thing. Probably different, but however... 😉

see #18

Samesame but different. E.g. you added sass indented, I only did scss.
On the other hand I did allot of other stuff...

However. Looks good, at first glance.

@tancredi
Copy link
Owner

Hi @janbiasi - thank you so much for the PR! It looks really good, though there are just a few things I'd like to change, one you may not like, so here it is:

While adding option maps for different asset types (pathOptions and formatOptions) I made a conscious choice not to make them available to the client, for the simple reason that I'd like to expand asset types quite a bit (defaults would probably remain just a few of the assets, but I'd like people to contribute more built-in templates etc etc.) and if we added that in it would just becomes a very fast increasing list (loads of options, loads to maintain)

I'd rather leave granular control to the config file, so you could specify options, custom paths and templates for each individual asset type only by defining it in JSON in the .fantasticonrc

How does that sound, reasonable?

@@ -11,6 +11,6 @@
"esModuleInterop": true
},
"typeRoots": ["node_modules/@types"],
"exclude": ["node_modules", "lib"],
"exclude": ["node_modules", "lib", "**/__tests__", "**/__mocks__"],
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes tests and mocks fallback to default TS config, which generates warnings and is quite unpredictable - would rather add this change it test files had their own configuration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, it would make sense to have a tsconfig.test.json for the test files; I've excluded the mocks and tests because they were also compiled and were included in the compiled output which isn't very great imo 😅

@tancredi tancredi merged commit 09b7678 into tancredi:master Nov 13, 2020
@tancredi
Copy link
Owner

For the time being I merged with a couple of changes (no cli options), so you can start using it but only with the config / API - but I'd like to keep the conversation open, it was more to get this out soon as it's very good stuff and other PRs depend on it :)

Available in version 1.0.14

@janbiasi
Copy link
Collaborator Author

Thanks a lot for the positive feedback, really appreciate it :) And as you mentioned, I totally forgot about the CLI options 🤦 my fault, sorry! And in regards of the tsconfig; I'll reply above to your feedback :)

@saiballo
Copy link

We use icon-font-generator in same bash scripts with cpoint option called in line command. In fantasticon I would have preferred to access it via CLI and not in a config file...

by the way you are the main dev :-)

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

4 participants