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

Add cosmetic improvements to the Webpacker guide #41165

Merged
merged 1 commit into from Jan 25, 2021

Conversation

davidstosik
Copy link
Contributor

@davidstosik davidstosik commented Jan 19, 2021

What

Small changes to the Webpacker guide, mainly to improve readability.

Why

I was reviewing #40817 but somehow dropped my review unfinished, and found some suggestions recently that I thought would be worth pushing.

Anything else?

@rossta did a great job documenting Webpacker, and I would like to thank him for it.
This was a great read and it clarified the blurry vision I had of Webpacker.
This PR only provides minor cosmetic changes that I didn't want to get lost in an unfinished PR review.
I'll provide a few in-line comments where deemed useful.

Sorry for the misleading tags! 🙇🏻‍♂️ I used GitHub's hub to submit my pull-request, and it looks like it defaulted to the master branch, introducing (a lot!) more changes in the diff than was planned.

@@ -22,9 +22,10 @@ Webpacker is a Rails wrapper around the [webpack](https://webpack.js.org) build

### What is webpack?

The goal of webpack, or any front-end build system, is to allow you to write your front-end code in a way that is convenient for developers and then package that code in a way that is convenient for browsers. With webpack, you can manage JavaScript, CSS, and static assets like files or fonts. Webpack will allow you to write your code, reference other code in your application, transform your code, and combine your code into easily downloadable packs.
The goal of webpack, or any front-end build system, is to allow you to write your front-end code in a way that is convenient for developers and then package that code in a way that is convenient for browsers. With webpack, you can manage JavaScript, CSS, and static assets like images or fonts. Webpack will allow you to write your code, reference other code in your application, transform your code, and combine your code into easily downloadable packs.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought "image" would be a more practical example of a "static asset", compared to "file".

@@ -137,7 +139,7 @@ If you are using a CSS framework, you can add it to Webpacker by following the i
### Using Webpacker for Static Assets

The default Webpacker [configuration](https://github.com/rails/webpacker/blob/master/lib/install/config/webpacker.yml#L21) should work out of the box for static assets.
The configuration includes several image and font file format extensions, allowing Webpack to include them in the generated `manifest.json` file.
The configuration includes several image and font file format extensions, allowing webpack to include them in the generated `manifest.json` file.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"webpack" is spelled without a capital everywhere else in this doc.

Copy link
Member

Choose a reason for hiding this comment

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

I see mostly capitalized usages outside of commands in the current doc. Please revert this change and let's capitalize it everywhere where it isn't a command (like we do with Active Record).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to clarify: "Webpacker" is indeed capitalized everywhere in this doc, but I had the impression webpack's official spelling does not take a capital letter, and all instances of the word in this file, except one, followed that.
Even in its official documentation, as the first word of a sentence, it is not capitalized.

Copy link
Member

Choose a reason for hiding this comment

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

🤦 I read this as webpacker the first time. My mistake, this makes sense, thanks for clarifying 😄

@@ -177,18 +179,15 @@ As of Webpacker version 5, Webpacker is not "engine-aware," which means Webpacke

Gem authors of Rails engines who wish to support consumers using Webpacker are encouraged to distribute frontend assets as an NPM package in addition to the gem itself and provide instructions (or an installer) to demonstrate how host apps should integrate. A good example of this approach is [Alchemy CMS](https://github.com/AlchemyCMS/alchemy_cms).

### Hot module replacement
### Hot Module Replacement (HMR)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mention of HMR below confused me at first, so I thought this may clarify it.


Don't forget to disable HMR if you are not running webpack-dev-server otherwise you will get not found error for stylesheets.
Don't forget to disable HMR if you are not running webpack-dev-server otherwise you will get "not found error" for stylesheets.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure where the quotes go here. Possibly:

Suggested change
Don't forget to disable HMR if you are not running webpack-dev-server otherwise you will get "not found error" for stylesheets.
Don't forget to disable HMR if you are not running webpack-dev-server otherwise you will get "not found" errors for stylesheets.

@rafaelfranca
Copy link
Member

There are conflicts on that file. Can you please rebase?

- fix typos like double spaces, accidental caps
- improve some HTML links by giving them a label
- etc
@davidstosik
Copy link
Contributor Author

@rafaelfranca Done!

@gmcgibbon gmcgibbon merged commit f15f494 into rails:main Jan 25, 2021
@davidstosik davidstosik deleted the webpacker-nitpicks branch January 26, 2021 00:05
@SuShu19 SuShu19 mentioned this pull request Feb 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants