modernizr-loader icon indicating copy to clipboard operation
modernizr-loader copied to clipboard

Improve documentation

Open jmarceli opened this issue 8 years ago • 9 comments

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

  • add info about .modernizrrc available options by linking https://github.com/Modernizr/Modernizr/blob/master/lib/config-all.json (like in https://www.npmjs.com/package/modernizr-webpack-plugin)
  • rearrange instructions. As it is a webpack loader I think that require and import code blocks should be placed after webpack.config example file with an information where you should write require code (it should be written in your code not in the webpack.config which may not be so obvious)

If you like this suggestions I may send a PR.

jmarceli avatar Jun 23 '16 14:06 jmarceli

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

flootr avatar Jun 27 '16 05:06 flootr

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.

goesbysteve avatar Jun 27 '16 07:06 goesbysteve

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 https://github.com/jmarceli/modernizr-loader-sage/commit/f895441cef232e957e0f5feaa2eafc01a64ca9fd

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

jmarceli avatar Jun 29 '16 14:06 jmarceli

@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'],
},
// ...

goesbysteve avatar Jun 29 '16 15:06 goesbysteve

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).

jmarceli avatar Jun 29 '16 16:06 jmarceli

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.

goesbysteve avatar Jun 29 '16 16:06 goesbysteve

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

Go for it 👍 @stevegibbings

flootr avatar Jun 29 '16 17:06 flootr

@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.

jmarceli avatar Nov 08 '16 11:11 jmarceli

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.

goesbysteve avatar Nov 10 '16 06:11 goesbysteve