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

Migration to Asset Pipeline #1907

Merged
merged 16 commits into from Apr 12, 2013
Merged

Migration to Asset Pipeline #1907

merged 16 commits into from Apr 12, 2013

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Apr 10, 2013

Rails 3.2 provides a way to incorporate assets within the project without needing an outside library (previously Jammit). This pull request migrates us off of Jammit and on to the Rails 3.2 asset pipeline with the following implications:

  • Jammit no longer required, no more config/assets.yml
  • All un-compiled assets (stylesheets, images, javascripts) now live under app/assets
  • No more Alchemy sub-module, instead it's used like an engine and installed like any other gem
  • References within views to stylesheets and javascript keep their form, just the location may have changed
  • During RPM build, a task to compile the assets is now required - 'rake assets:precompile' (see config/application.rb to see how what is compiled is determined)
  • Compiled, production assets still live at public/assets
  • Some assets live under vendor/assets
  • Comps updates for the new gems
  • New Gems: compass-rails, sass-rails, ui_alchemy-rails, uglifier and therubyracer (in development only)

Testing of this work:

@daviddavis
Copy link
Contributor

A couple minor concerns. First, all your copyrights still say 2012 and I think that was recently updated? Also, one of my concerns is that we're not using the asset pipeline for what it's intended. It's suppose to compile all the javascript assets together to speed up requests via caching and instead we're creating separate javascript files for each page.

else
false
end
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing you used curly braces here because you ran into presedence problems with do..end? If so, you could use parentheses which would allow you to use do end (which is typically recommended for multi-line blocks of code).

@ehelms
Copy link
Member Author

ehelms commented Apr 11, 2013

Depending on who you ask, one file vs. lots of files is debated hotly. The only consensus I have ever read from people that you want a balance between the size of your JS files and the number of JS files to prevent a degradation to the user experience. I am not shrugging it off as shouldn't be investigated, but I don't think this pull request should necessarily address that given our time frame. The production installs I have used so far have been blazing fast with no impact to user experience as of yet.

Since @daviddavis and @waldenraines have raised this as a concern and if they are agreeable, I'll add backlog item to investigate this for a future sprint?

@daviddavis
Copy link
Contributor

+1

@ehelms
Copy link
Member Author

ehelms commented Apr 11, 2013

@daviddavis tracked here https://trello.com/card/investigate-ui-asset-organization/511bd7357c52d1c61c00510e/68

FYI: New navigation and nutupane are introducing Angular as a core library that components and UI are built off of.

@ehelms
Copy link
Member Author

ehelms commented Apr 12, 2013

@daviddavis Updated with your comments, please review

@@ -0,0 +1,17 @@
/**
Copyright 2012 Red Hat, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still seeing 2012 in a lot of files.

@daviddavis
Copy link
Contributor

ACK pending copyright fix.

@ehelms
Copy link
Member Author

ehelms commented Apr 12, 2013

@daviddavis updated copyright years

@daviddavis
Copy link
Contributor

ACK

ehelms added a commit that referenced this pull request Apr 12, 2013
@ehelms ehelms merged commit 5689fc8 into Katello:master Apr 12, 2013
ehelms added a commit that referenced this pull request Apr 27, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants