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

Assets documentation #2418

Merged
merged 4 commits into from
Jan 2, 2018
Merged

Conversation

benjaminwil
Copy link
Contributor

I’ve rewritten and expanded the documentation about assets in Solidus. I also split it into two articles.

The changes aren't incredibly significant, but here's a summary:

  • I link back to relevant Rails documentation about manifests and the standard asset pipeline.
  • When referencing the Solidus source in examples, I've made sure to update it to code that actually makes sense in master. (Previous references wouldn't have made sense if you looked at the corresponding source now.)
  • I've highlighted how we recommend scoping assets to frontend and backend so nothing conflicts and how we don't really recommend overriding in the long term.
  • I've stated clearly that the articles assume a typical Solidus installation (with solidus_frontend and solidus_backend).

In the future, I'd like to see more information about Solidus's asset manifests added.

This is part a larger project to improve Solidus's documentation. See this gist with the high-level table of contents. Where and how this documentation will exist is still up for discussion.

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Great. Only have some small nits :)

`app/assets` tree.
- Manifests (the `all.css`, `all.js`, and `application.js` files in your
project's `assets` trees) requires your app's external libraries or custom
assets – including any any files or directories you add deeper in the
Copy link
Member

Choose a reason for hiding this comment

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

any any


```
app|vendor
|-- assets
Copy link
Member

Choose a reason for hiding this comment

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

I know this very picky, but could we use box drawing characters here? 😄

app|vendor
└─ assets
   ├─ images
   │  └─ spree
   │     ├─ frontend
   │     └─ backend
   ├─ javascripts
   │  └─ spree
   │     ├─ frontend
   │     └─ backend
   └─ stylesheets
      └─ spree
         ├─ frontend
         └─ backend

| |-- all.css
|-- backend
|-- all.css
```
Copy link
Member

Choose a reason for hiding this comment

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

vendor
└─ assets
   ├─ javascripts
   │  └─ spree
   │     ├─ frontend
   │     │  └─ all.js
   │     └─ backend
   │        └─ all.js
   └─ stylesheets
      └─ spree
         ├─ frontend
         │  └─ all.css
         └─ backend
            └─ all.css

I ❤️ box drawing characters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

❤️ Agreed

`spree/frontend` or `spree/backend` subdirectories to avoid conflicts or
accidental file overrides.

For example, if you want to use the [Foundation CSS framework][foundation] in
Copy link
Member

Choose a reason for hiding this comment

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

Missing link

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 added this at the bottom of the section. Thank you.

I fixed a typo, added a missing link, and now use box drawing
characters for tree diagrams.
@benjaminwil
Copy link
Contributor Author

@tvdeyen Hey, I updated the asset management guide per your review. When you have a chance could you re-review?

@jhawthorn jhawthorn merged commit 7768463 into solidusio:master Jan 2, 2018
@benjaminwil benjaminwil deleted the assets_documentation branch April 30, 2018 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants