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

[don't merge] Refactor CSS assets to use Sprockets instead of Webpack #1864

Closed
wants to merge 33 commits into from

Conversation

Bodacious
Copy link
Contributor

@Bodacious Bodacious commented Sep 6, 2018

Fixes #1862, #1762

Changes proposed in this PR:

  • Move CSS files into app/assets
  • Use Sprockets for SCSS compilation
  • Update rake assets:precompile to support JS and CSS

Copy link
Contributor

@briri briri left a comment

Choose a reason for hiding this comment

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

Changes look ok. Its really hard to tell though if there are any visual issues with the CSS reorg without running and visually inspecting the site.

Why move back over to Sprockets (aside from being more Rails-ish)? What advantage are we gaining? We would still need to support Webpack to handle our ES6 JS.

@jollopre
Copy link
Contributor

jollopre commented Sep 6, 2018

If I remember well, we decided to move towards npm to handle front-end related issues since they are more actively maintained than front-end assets handled through ruby gems.

Although these changes certainly keep the codebase more Rails focused, I got my doubts whether or not it is an appropriate move knowing that Rails 5 introduced Webpack on their assets pipeline...

Copy link
Contributor

@xsrust xsrust left a comment

Choose a reason for hiding this comment

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

Really like the refactoring out of variables and hierarchical layout.

I'll need to have a look through all the customization we're using and make sure everything we're changing is captured in the variables.

@Bodacious
Copy link
Contributor Author

Readme should be updated to include:

Assets Management

Javascripts

Javascript are written using ES6 and can be found in app/assets/javascripts. The application uses the recommended configuration for using Webpack in Rails 5: the webpacker gem.

You don't have to run a separate process for Webpack for JS to be compiled. This is compiled automatically on each request (in development).

Be sure to check the Rails log if you are seeing unexpected Javascript behaviour. There's an ESLint check run each time the assets are compiled, and errors are reported in the Rails log.

Stylesheets, images and other assets

Stylesheets are written using Sassy CSS (via lib-c SASS)

Stylesheets use Rails' asset pipeline for compilation. For more information on how that is configured, please read this guide: https://guides.rubyonrails.org/asset_pipeline.html

Compiling

To compile assets for development, please use the rake task rake assets:precompile

@briri
Copy link
Contributor

briri commented Sep 10, 2018

👍 The changes look fine to me. I will need to do some updating of the customized CSS for the DMPTool once this is in place.

It would be good to write up some recommendations/suggestions for how to go about customizing the CSS for local installations.

Feel free to merge into dev while I'm away

@Bodacious Bodacious changed the title Refactor CSS assets to use Sprockets instead of Webpack [don't merge] Refactor CSS assets to use Sprockets instead of Webpack Sep 10, 2018
@Bodacious Bodacious force-pushed the refactor/css-assets branch 2 times, most recently from fc9eefc to c102270 Compare September 13, 2018 10:36
@Bodacious Bodacious changed the base branch from development to next-sprint September 13, 2018 11:17
@xsrust
Copy link
Contributor

xsrust commented Oct 15, 2018

@Bodacious as this is pointed at the next-sprint branch, is there still reason not to merge it into that branch?

@Bodacious
Copy link
Contributor Author

@xsrust Let me have another look at it and make sure it's ready.

@Bodacious
Copy link
Contributor Author

Please ignore this. Prefer #1962

@briri
Copy link
Contributor

briri commented Oct 23, 2018

would suggest just closing this PR in favor of #1962

@Bodacious Bodacious closed this Oct 30, 2018
@briri briri deleted the refactor/css-assets branch December 9, 2019 17:06
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.

4 participants