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 Asset Pipeline documentation [ci-skip] #44482

Merged
merged 1 commit into from Feb 21, 2022

Conversation

la-ruby
Copy link
Contributor

@la-ruby la-ruby commented Feb 18, 2022

Summary

  • Saw some room for improvement in this part of the documentation:

Before

before

After

after

@rails-bot rails-bot bot added the docs label Feb 18, 2022
Instead of returning a path such as `/assets/smile.png` (digests are left out
for readability). The URL generated will have the full path to your CDN.
The URL generated will have the full path to your CDN instead of having a path
such as `/assets/smile.png` (digests are left out for readability):
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the original text was supposed to conclude the text above it, but, even after your edit, I had some trouble understanding line 868.

What do you think about the following?

--- a/guides/source/asset_pipeline.md
+++ b/guides/source/asset_pipeline.md
@@ -854,23 +854,19 @@ config.asset_host = ENV['CDN_HOST']
 NOTE: You would need to set `CDN_HOST` on your server to `mycdnsubdomain
 .fictional-cdn.com` for this to work.

-Once you have configured your server and your CDN when you serve a webpage that
-has an asset:
+Once you have configured your server and your CDN, asset paths from helpers such
+as:

 ```erb
 <%= asset_path('smile.png') %>
 ```

-Instead of returning a path such as `/assets/smile.png` (digests are left out
-for readability). The URL generated will have the full path to your CDN.
+Will be rendered as full CDN URLs like `http://mycdnsubdomain.fictional-cdn.com/assets/smile.png`
+(digest omitted for readability).

-```
-http://mycdnsubdomain.fictional-cdn.com/assets/smile.png
-```
-
-If the CDN has a copy of `smile.png` it will serve it to the browser and your
-server doesn't even know it was requested. If the CDN does not have a copy it
-will try to find it at the "origin" `example.com/assets/smile.png` and then store
+If the CDN has a copy of `smile.png`, it will serve it to the browser, and your
+server doesn't even know it was requested. If the CDN does not have a copy, it
+will try to find it at the "origin" `example.com/assets/smile.png`, and then store
 it for future use.

 If you want to serve only some assets from your CDN, you can use custom `:host`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great! you've improved the readability even more

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added ffeba0d

@jonathanhefner
Copy link
Member

@la-ruby That looks good! Would you mind also squashing the commits?

- A dot was appearing in what was supposed to be a compound statement
@la-ruby
Copy link
Contributor Author

la-ruby commented Feb 21, 2022

@la-ruby That looks good! Would you mind also [squashing the commits]

squashed @jonathanhefner

@jonathanhefner jonathanhefner merged commit 54a2824 into rails:main Feb 21, 2022
@jonathanhefner
Copy link
Member

jonathanhefner commented Feb 21, 2022

Thank you, @la-ruby! 🎉

Backported to 7-0-stable in 631e5aa.

jonathanhefner added a commit to jonathanhefner/rails that referenced this pull request Feb 21, 2022
Improve Asset Pipeline documentation [ci-skip]

(cherry picked from commit 54a2824)
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