Adjusted Asset Management #1305

TheDutchCoder · 2017-06-19T15:02:01Z

Tried to make the guide as a follow up to Getting Started.
Changed some wording and added a lot of output examples, diffs, and sources.

Tried to make the guide as a follow up to `Getting Started`. Changed some wording and added a lot of output examples, diffs, and sources.

* Update `src/index.js` to not import anything.

* Added missing semi colons

Typo in diff and some style changes

skipjack

This looks pretty good, see my comments. I am a bit worried about how much is being added here but I'm not sure I see a better way. Interested in what the rest of the @webpack/documentation-team thinks.

The problem is that the more detailed things get, the more updates will be required to maintain all of it. For example, the webpack version and output format could quickly become dated (v3 just came out). Though I guess for most of these core features the usage shouldn't change too drastically.

skipjack · 2017-06-19T23:07:24Z

content/guides/asset-management.md

 ---

-So you're all set up with webpack -- transforming and linting your JavaScript modules, generating an html file with the [`HTMLWebpackPlugin`](/plugins/html-webpack-plugin), and even loading some css through your JavaScript modules with the [css-loader](/loaders/css-loader). But wait, your site requires a whole bunch of other assets like images (e.g. `.png`, `.jpg`, `.svg`), fonts (e.g. `.woff`, `.woff2`, `.eot`), and data (e.g. `.json`, `.xml`, `.csv`)!
+If you've been following the guides fromt he start, you will now have a small project that shows "Hello webpack", but it would be nice to make it a little more complex by adding a couple of different assets and see how webpack handles these.


/s/fromt he/from the

Also I would change:

... "Hello webpack", but it would be nice to make it a little more complex by adding a couple of different assets and see how webpack handles these.

to something like:

... "Hello webpack". Now let's try to incorporate some other assets, like images, to see how they can be handled.

skipjack · 2017-06-19T23:26:54Z

content/guides/asset-management.md


 ``` bash
 npm install --save-dev style-loader css-loader
 ```

+Now open your `weback.config.js` file that we created earlier and adjust it so that we can tell webpack to also handle our CSS files.
+


I think you can leave this line out. The instructions above about adding them to the module configuration should be enough.

skipjack · 2017-06-20T02:29:07Z

content/guides/asset-management.md

+   [8] ./src/index.js 351 bytes {0} [built]
+```
+
+Open up `index.html` in your browser again and you should see that `Hello webpack` is now styled in red. If you want to see what webpack did, inspect the page (don't view the page source, as it won't show you the result) and look at the page's head tags. It should contain our style block that we imported in `index.js`:


I would change:

If you want to see what webpack did, inspect...

to

To see what webpack did, inspect...

skipjack · 2017-06-20T02:32:49Z

content/guides/asset-management.md

+    <div class="hello">Hello webpack</div>
+  </body>
+</html>
+```


I think you can drop this example. Instead maybe just mention that the style-loader dynamically inserts <style> blocks.

skipjack · 2017-06-20T02:35:32Z

content/guides/asset-management.md

-  //...
+  }
+};
+


Drop the empty line (198).

skipjack · 2017-06-20T03:20:34Z

content/guides/asset-management.md



 ## Loading Fonts

-So what about other assets like fonts? The file and url loaders will take any file you load through them and output it to your build directory. This means we can use them for any kind of file, including fonts:
+So what about other assets like fonts? The file and url loaders will take any file you load through them and output it to your build directory. This means we can use them for any kind of file, including fonts. Let's adjust our `webpack.config.js` file to instruct it to handle font files:


I would simplify:

Let's adjust our `webpack.config.js` file to instruct it to handle font files:

to

Let's update our `webpack.config.js` to handle font files:

skipjack · 2017-06-20T03:24:47Z

content/guides/asset-management.md

+   |- index.html
+ |- /src
+   |- MyFont.woff
+   |- MyFont.woff2


This is somewhat nit-picky but I would suggest sticking with lower cased file names so either font.woff/font.woff2 or my-font.woff/my-font.woff2.

If you do make this change, make sure to change them below as well.

Good catch, will do!

skipjack · 2017-06-20T03:28:18Z

content/guides/asset-management.md

+ |- /node_modules
+```
+
+With the loader configured and the fonts in place, you can use the fonts in `@font-face` declarations like you're used to. webpack will automatically serve these files to the loader, just like it did with the images:


I would rewrite as:

With the loader configured and fonts in place, you can use incorporate them via an `@font-face` declaration. The local `url(...)` directive will be picked up by webpack just as it was with the image:

skipjack · 2017-06-20T03:29:42Z

content/guides/asset-management.md

}
+

I think the trailing newline can be dropped here as well.

👍 It's my editor taking over ;)

skipjack · 2017-06-20T03:31:21Z

content/guides/asset-management.md



 ## Loading Data

-Another useful asset that can be loaded is data, like JSON files, CSVs, TSVs, and XML. Support for JSON is actually built-in, similar to NodeJS, meaning `import Data from './data.json'` will work by default. To import CSVs, TSVs, and XML you could use the [csv-loader](https://github.com/theplatapi/csv-loader) and [xml-loader](https://github.com/gisikw/xml-loader). Let's handle loading all three:
+Another useful asset that can be loaded is data, like JSON files, CSVs, TSVs, and XML. Support for JSON is actually built-in, similar to NodeJS, meaning `import MyData from './data.json'` will work by default. To import CSVs, TSVs, and XML you could use the [csv-loader](https://github.com/theplatapi/csv-loader) and [xml-loader](https://github.com/gisikw/xml-loader). Let's handle loading all three:


Again, not sure if the variable name change is necessary.

In this case it won't matter probably, I'l revert it.

* Made all requested changes * Tested `Image` as a variable and changed variable name * Changed diffs to line up * Minor tweaks to be more consistent with `Getting Started`

TheDutchCoder · 2017-06-20T12:07:12Z

@skipjack made all the adjustments and double checked the Image variable (it will produce an error), can you review again? 👍

* Changed `component ()` to `component()`

skipjack · 2017-06-21T12:39:14Z

content/guides/asset-management.md

 ```

-and the relative paths (e.g. `'./font.woff2'`) will be replaced with the final path/filename in your build directory.
+Open up `index.html` again and see if our `Hello webpack` text has changed to the new font. If all is well, you should see the changes. Awesome! This is really starting to come together now, isn't it?


I think you can probably drop the last line here:

Awesome! This is really starting to come together now, isn't it?

or at least the Awesome! part, but I'll leave that up to you.

skipjack · 2017-06-21T12:40:26Z

content/guides/asset-management.md

+      ]
+    }
+  };
+


I would drop this trailing line -- maybe we can add something to our linter to flag on these.

skipjack · 2017-06-21T12:42:24Z

content/guides/asset-management.md

 ```

-Now you can `import` any one of those four types of data (JSON, CSV, TSV, XML) and the `Data` variable you import it to will contain parsed JSON for easy consumption:
+Now you can `import` any one of those four types of data (JSON, CSV, TSV, XML) and the `MyData` variable you import it to will contain parsed JSON for easy consumption:


This doesn't correspond with the line above where Data is used as the variable... I would either drop this change or change them both to MyData.

Sorry, completely missed that, should be fixed!

* Changed `MyData` back to `Data`

skipjack

All in all this looks great -- see the last few comments I added.

* Dropped trailing newline * Removed some extraneous text

skipjack · 2017-06-21T13:06:00Z

Thanks again for your work on this! Will merge soon. Want to tackle output-management.md next? I'll dig into all the code-splitting and lazy load guides this weekend and will ping you for review. I'm also planning to get rid of a few that only have TODOs in them and potentially move one or two (like comparison) out of the /guides. I'm really happy with the way this is progressing and I hope you are too!

TheDutchCoder · 2017-06-21T13:31:23Z

No problem man, it's a very enjoyable learning experience!

Yeah I'll hop onto Output Management next, I'll probably ping you quite a few times for that one if that's alright. I'll keep the fork until it's ready and then I can submit a PR, should be a bit cleaner as opposed to my patching the PR all the time ;)

I'm certainly happy the way this is going and I hope that beginners will find it easier to follow, which is ultimately our goal as well!

* Removed newline

TheDutchCoder · 2017-06-21T13:45:05Z

Not sure what the linter failed on tbh. Fixed a new line issue while I was at it.

skipjack · 2017-06-21T13:46:28Z

It was some timeouts on hyperlinks -- not anything caused by you. I restarted the build -- we run into that every so often, but it should run ok this time (at least in terms of link testing).

TheDutchCoder · 2017-06-21T15:12:29Z

Failed again :/

skipjack · 2017-06-21T15:16:13Z

Ok don't worry about it, either way I'll get this sorted out. If any testing errors crop up we can always address in a separate PR. Our tests just ping the hell out of img.shields.io currently, we may try to filter those out from our link testing process.

TheDutchCoder · 2017-06-21T15:19:50Z

Alright cool, I'll await the merge ;)

TheDutchCoder added 4 commits June 19, 2017 10:40

Adjusted Asset Management

fbb7687

Tried to make the guide as a follow up to `Getting Started`. Changed some wording and added a lot of output examples, diffs, and sources.

Forgot to remove stuff from index.js

1b54224

* Update `src/index.js` to not import anything.

Updated asset-management.md

63167b6

* Added missing semi colons

Updated asset-management.md

4b0e5c6

Typo in diff and some style changes

skipjack added Guides Enhancement labels Jun 19, 2017

skipjack mentioned this pull request Jun 20, 2017

Guides - Review and Simplify #1258

Closed

skipjack requested changes Jun 20, 2017

View reviewed changes

Updated asset-management.md

423d382

* Made all requested changes * Tested `Image` as a variable and changed variable name * Changed diffs to line up * Minor tweaks to be more consistent with `Getting Started`

TheDutchCoder added 2 commits June 20, 2017 08:19

Updated asset-management.md

89f88dc

* Changed `component ()` to `component()`

Merge branch 'master' into patch-3

d0673c8

skipjack reviewed Jun 21, 2017

View reviewed changes

Updates asset-management,md

daaff49

* Changed `MyData` back to `Data`

skipjack approved these changes Jun 21, 2017

View reviewed changes

Updates asset-management.md

d17024c

* Dropped trailing newline * Removed some extraneous text

Updated asset-management.md

dcb5c8d

* Removed newline

Merge branch 'master' into patch-3

f3a8a64

skipjack merged commit 5a5f1a2 into webpack:master Jun 21, 2017

Adjusted Asset Management #1305

Adjusted Asset Management #1305

Conversation

TheDutchCoder commented Jun 19, 2017

skipjack left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheDutchCoder commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skipjack left a comment

Choose a reason for hiding this comment

skipjack commented Jun 21, 2017

TheDutchCoder commented Jun 21, 2017

TheDutchCoder commented Jun 21, 2017

skipjack commented Jun 21, 2017

TheDutchCoder commented Jun 21, 2017

skipjack commented Jun 21, 2017

TheDutchCoder commented Jun 21, 2017

skipjack left a comment •

edited

Loading