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

Improve documentation #12

Open
jmarceli opened this issue Jun 23, 2016 · 9 comments
Open

Improve documentation #12

jmarceli opened this issue Jun 23, 2016 · 9 comments

Comments

@jmarceli
Copy link
Contributor

Hi,
This is really useful project thanks for that. Anyway I would like to suggest some README improvements:

If you like this suggestions I may send a PR.

@flootr
Copy link
Contributor

flootr commented Jun 27, 2016

Hey! Thanks for your suggestions. Feel free to send a PR 👍

@goesbysteve
Copy link

I'm happy to help too as I stumbled a bit on not setting "setClasses: true". Without that I was not getting all css classes added for my tests.

@jmarceli
Copy link
Contributor Author

As I finished my minimal example for #13 I think I may share it as a reference for other WP Sage theme users. Here is my repo and specific example commit which adds modernizr-loader jmarceli/modernizr-loader-sage@f895441

What do you think? Should I modify README or place it in the Wiki or maybe leave it alone :)

@goesbysteve
Copy link

@flootr I'd like to add to the README to include the option of adding modernizr to an entry point in webpack.config.js if only used in CSS and therefore importing into modules isn't necessary. For those just using it for CSS this is far simpler and avoids an unused import or finding an arbitrary place to require Modernizr.

// ...
entry: {
    main: ['modernizr', './your/javascript/modules'],
},
// ...

@jmarceli
Copy link
Contributor Author

jmarceli commented Jun 29, 2016

This sounds like an useful info for me but it should be somehow separated from base configuration in order not to mislead people.
Maybe new section name e.g. Alternative configuration, and current would be named Base configuration (or the other way round).

@goesbysteve
Copy link

Ok why don't I do a pull request for what I've done and we can iterate in that?

Sent from my iPhone

On 29 Jun 2016, at 17:45, jmarceli [email protected] wrote:

This sounds like an useful info for me but it should be somehow separated from base configuration in order not to mislead people.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

@flootr
Copy link
Contributor

flootr commented Jun 29, 2016

Ok why don't I do a pull request for what I've done and we can iterate in that?

Go for it 👍 @stevegibbings

@jmarceli
Copy link
Contributor Author

jmarceli commented Nov 8, 2016

@stevegibbings investigating further it is even easier to import 'modernizr' than modify webpack config file. I think that the result is similar but this way if we need it in JS it is enough to change import 'modernizr' to import Modernizr from 'modernizr'.
I see no drawbacks of that approach and I would favour it over modifying a config file.

@goesbysteve
Copy link

Yes I can see how that would work.

Sent from my iPhone

On 8 Nov 2016, at 11:51, jmarceli [email protected] wrote:

@stevegibbings investigating further it is even easier to import 'modernizr' than modify webpack config file. I think that the result is similar but this way if we need it in JS it is enough to change import 'modernizr' to import Modernizr from 'modernizr'.
I see no drawbacks of that approach and I would favour it over modifying a config file.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

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

No branches or pull requests

3 participants