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

Fix confusing wording in Asset Pipeline guide [ci skip] #38782

Merged
merged 6 commits into from Mar 24, 2020

Conversation

rmacklin
Copy link
Contributor

Summary

While looking into #38307, I noticed some confusing wording in the Asset Pipeline guide:

Rails automatically adds the `sass-rails` gem to your `Gemfile`, which is used
by Sprockets for asset compression:
```ruby
gem 'sass-rails'
```
Using the `--skip-sprockets` option will prevent Rails from adding
them to your `Gemfile`, so if you later want to enable
the asset pipeline you will have to add those gems to your `Gemfile`. Also,

Specifically

adding them to your Gemfile

and

add those gems

is confusing because the preceding paragraph only mentions one gem (sass-rails). I discovered that this is a result of removing mentions of uglifier and coffee-rails from that paragraph in #35994.

So I've opened this PR as a follow-up to #35994 to clean up the confusing wording that was left in the guide.

Other Information

While I was in there, I also clarified the purpose of the sass-rails gem and added links to https://github.com/rails/sass-rails and https://sass-lang.com.

Prior to ab123a3, this guide mentioned
that three gems (sass-rails, uglifier, and coffee-rails) would be added
to the Gemfile by default, unless the `--skip-sprockets` option was
used when generating the application.
However, ab123a3 removed uglifier and
coffee-rails from the guide (to reflect the switch to webpacker for
managing javascript in rails 6). Now that only sass-rails is left, the
guide should say "adding this to your Gemfile" instead of "adding them
to your Gemfile" and "add that gem" instead of "add those gems".

[ci skip]
The original text "used by Sprockets for asset compression" was written
when this sentence was summarizing the purpose of three gems:
sass-rails, uglifier, and coffee-rails.
However, ab123a3 removed uglifier and
coffee-rails from this sentence. Now that only sass-rails is left, we
can be more specific about what it is used for: compiling Sass
stylesheets.

[ci skip]
We link to the sprockets-rails repository in the preceding paragraph,
so it seems appropriate to also link to the sass-rails repository in
this paragraph. And since readers of this guide may not know what Sass
is, it also seems appropriate to link to the sass-lang.com website.

[ci skip]
Copy link
Member

@eugeneius eugeneius left a comment

Choose a reason for hiding this comment

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

There are a few more outdated references in the Preprocessing section, in particular the assertion that Rails generates a CoffeeScript file with every scaffold by default. Could you do a pass over those too?

guides/source/asset_pipeline.md Outdated Show resolved Hide resolved
rmacklin and others added 2 commits March 22, 2020 17:23
[ci skip]

Co-Authored-By: Eugene Kenny <elkenny@gmail.com>
of the Asset Pipeline guide

This section previously declared that the Rails controller generator
generated an `app/assets/javascripts/projects.coffee` and an
`app/assets/stylesheets/projects.scss` file. Then, in
ab123a3, we removed the assertion that
`app/assets/javascripts/projects.coffee` would be generated (to reflect
the switch to webpacker for managing javascript in Rails 6). However,
the preceding sentence and the subsequent paragraph were still written
with the assumption that a CoffeeScript file would be generated.

Now we've reworded those parts to be consistent with the fact that Rails
no longer generates a CoffeeScript file.

[ci skip]
@rmacklin
Copy link
Contributor Author

Sounds good, I've updated the Preprocessing section in e71692f

`app/assets/stylesheets/projects.scss` file.

In development mode, or if the asset pipeline is disabled, when this file is
requested it is processed by the processor provided by the `sass` gem and then
Copy link
Member

Choose a reason for hiding this comment

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

The sass gem is no longer maintained, and sass-rails now depends on the sassc gem instead via sassc-rails (#32896).

Even if it's not technically accurate, I think this should say "provided by the sass-rails gem" - for the purposes of following this guide, it's enough to know that the processor is available because sass-rails is in the Gemfile, even if the code is ultimately provided by a different gem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Updated to sass-rails in 649d265

As of rails/sass-rails@409d871
sass-rails no longer uses the `sass` gem (which is no longer
maintained; it has been replaced by the `sassc` gem). Thus, the rails
guides should not mention the `sass` gem specifically. Instead, they can
continue to reference the `sass-rails` gem.

[ci skip]
@eugeneius
Copy link
Member

Could you squash your commits? Otherwise I think this is ready to go.

@rmacklin
Copy link
Contributor Author

Can you do a squash-and-merge? Or do you want me to squash into more than 1 commit?

@eugeneius
Copy link
Member

If I use the squash and merge button, the autogenerated commit message will be enormous:

Fix confusing wording in Asset Pipeline guide [ci skip]

* Clean up leftover plural references in Asset Pipeline guide

Prior to ab123a33d2460dcdc5c36001cef5316eadc75fb3, this guide mentioned
that three gems (sass-rails, uglifier, and coffee-rails) would be added
to the Gemfile by default, unless the `--skip-sprockets` option was
used when generating the application.
However, ab123a33d2460dcdc5c36001cef5316eadc75fb3 removed uglifier and
coffee-rails from the guide (to reflect the switch to webpacker for
managing javascript in rails 6). Now that only sass-rails is left, the
guide should say "adding this to your Gemfile" instead of "adding them
to your Gemfile" and "add that gem" instead of "add those gems".

[ci skip]

* Clarify what sass-rails is used for in Asset Pipeline guide

The original text "used by Sprockets for asset compression" was written
when this sentence was summarizing the purpose of three gems:
sass-rails, uglifier, and coffee-rails.
However, ab123a33d2460dcdc5c36001cef5316eadc75fb3 removed uglifier and
coffee-rails from this sentence. Now that only sass-rails is left, we
can be more specific about what it is used for: compiling Sass
stylesheets.

[ci skip]

* Link to the sass-rails repo and sass-lang.com in Asset Pipeline guide

We link to the sprockets-rails repository in the preceding paragraph,
so it seems appropriate to also link to the sass-rails repository in
this paragraph. And since readers of this guide may not know what Sass
is, it also seems appropriate to link to the sass-lang.com website.

[ci skip]

* Further reword Asset Pipeline guide introduction to be less repetitive

[ci skip]

Co-Authored-By: Eugene Kenny <elkenny@gmail.com>

* Remove mention of generating CoffeeScript files in Preprocessing section

of the Asset Pipeline guide

This section previously declared that the Rails controller generator
generated an `app/assets/javascripts/projects.coffee` and an
`app/assets/stylesheets/projects.scss` file. Then, in
ab123a33d2460dcdc5c36001cef5316eadc75fb3, we removed the assertion that
`app/assets/javascripts/projects.coffee` would be generated (to reflect
the switch to webpacker for managing javascript in Rails 6). However,
the preceding sentence and the subsequent paragraph were still written
with the assumption that a CoffeeScript file would be generated.

Now we've reworded those parts to be consistent with the fact that Rails
no longer generates a CoffeeScript file.

[ci skip]

* Remove mention of the deprecated `sass` gem from Asset Pipeline guide

As of https://github.com/rails/sass-rails/commit/409d871c2a971691eac86c5a32e614e2941c72b8
sass-rails no longer uses the `sass` gem (which is no longer
maintained; it has been replaced by the `sassc` gem). Thus, the rails
guides should not mention the `sass` gem specifically. Instead, they can
continue to reference the `sass-rails` gem.

[ci skip]

Co-authored-by: Eugene Kenny <elkenny@gmail.com>

This is way too much detail for the patch it describes. I could edit it down myself, but I'd prefer if you handled it by squashing the commits and writing a shorter combined commit message.

@rmacklin
Copy link
Contributor Author

rmacklin commented Mar 23, 2020

I can squash and push later when I return to my rails dev machine. (Unfortunately, recloning on my current connection is not feasible.)

Alternatively, if you want to squash-merge now, here's a proposal for an abbreviated commit message:

Fix confusing wording in Asset Pipeline guide

* Clean up leftover plural "gems" references as a follow-up to
  ab123a33d2460dcdc5c36001cef5316eadc75fb3 (#38307)
* Remove mention of generating CoffeeScript files in Preprocessing section
* Clarify what sass-rails is used for
* Link to the sass-rails repo and sass-lang.com
* Remove mention of the deprecated `sass` gem to reflect the change from
  https://github.com/rails/sass-rails/commit/409d871c2a971691eac86c5a32e614e2941c72b8

[ci skip]

@eugeneius eugeneius merged commit 0c8206b into rails:master Mar 24, 2020
@eugeneius
Copy link
Member

Perfect, thanks! 💛 I squash-merged with that message.

@rmacklin rmacklin deleted the pr-35994-follow-up branch March 24, 2020 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants