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

New minimigration PR, just to take out all old formatting and make it all-bootstrap #337

Merged
merged 6 commits into from Feb 1, 2013

Conversation

Projects
None yet
3 participants
@rewritten
Collaborator

rewritten commented Jan 13, 2013

It will take some js-magic too, because there is a lot of Prototype left. In the end the targets are:

  • to have almost no app-specific css (use bootstrap plus font-awesome and for the missing parts, base on compass. This leads to a lesser weight (as bootstrap and font-awesome are served from CDN) and less maintenance overhead.
  • to delegate that most JavaScript magic is from either rails-ujs (using data-* attributes) or vendored jquery plugins (i.e., bootstrap's tabs and tooltips, select2, timeago...)
  • to have cleaner ERB templates, avoiding messy helpers in favour of explicit HTML and templates.

rewritten added some commits Jan 11, 2013

Added CDN-served bootstrap files (and jquery too).
Fixed tabs/panes in home and group views, because bootstrap is simpler and it works.
Bootstrappized the group home and edit form.
Vendorized jquery.timeago and declared on the whole app
Added forum's miniform inline
Implicit i18n paths in view
@acao

This comment has been minimized.

acao commented Jan 13, 2013

Ok, looks good! Only problem is that I plan on using bootstrap-sass because of the way I usually use bootstrap, so I will be forking this, but thanks this will work great! I should be able to jump in later this week!

@rewritten

This comment has been minimized.

Collaborator

rewritten commented Jan 14, 2013

@acao just as a reminder, bootstrap is not meant to be "inherited", there are a number of rules that match things like span* or icon-* so you can't safely use them in an @extend instruction. Moreover it completely defeats the main advantage of using a standard and common dependency, that your users will probably have a cached copy of the file from somewhere else (or just from the previous version of the app)

@acao

This comment has been minimized.

acao commented Jan 14, 2013

Indeed, I know just what you mean. So why do you think rails admin hasn't taken on this approach? I would be glad to contribute using the approach you describe, but I would much prefer to use a different one for our project. I'm used to using the bootstrap variables, mixins, etc. to build out new components and work with bootstrap in general. I'll be minifying/gzipping everything as well. Because our user cases are defined by people using it fairly regularly, rather than a lots of temporary unique visitors, we don't have as much of an issue with a long initial load time.

@rewritten

This comment has been minimized.

Collaborator

rewritten commented Jan 14, 2013

@acao There may be multiple reasons, sferik is an awesome programmer and I'm not in a position to criticize. Anyway take into account that:

  1. NetDNA servers were not distributing bootstrap at the time rails_admin project started, so it was not a choice
  2. Bootstrap is built with LESS, not with SASS, the bootstrap-sass gem is a port. Sass is much more powerful than LESS, and renders to better optmized code (hey, that's my opinion after reading a couple of articles and checking out a couple of projects, don't take it as a biblic statement). In any case having to port from one language to another may lead to errors and missing features
  3. In a way, you can configure bootstrap the same way you do with jQueryUI, so you can get (online) just the package you want, by joining just the required elements.
  4. Nothing prevents you to use sass in your css, and use the same variable values that you used to configure bootstrap in the rest of your code.

In the end, it's good to have the possibility of having theming applied to the project, by means of bootstrap configuration. One possible way to achieve it is to have a runtime option to either serve the standard CDN-hosted bootstrap, or another customized one. This could depend on any runtime condition, the current user, etc... It would be a vey welcome contribution, and will avoid infinitely splitting the project.

About mixins, the few usable mixins that you can find in bootstrap-sass are completely covered by compass, and compass does not impose any single class on your css. You just use the mixins when needed.

@herestomwiththeweather herestomwiththeweather merged commit 5501931 into rails329 Feb 1, 2013

@rewritten rewritten deleted the bootstrap222 branch Feb 1, 2013

@acao

This comment has been minimized.

acao commented Feb 15, 2013

I don't see this in the rails329 branch... am I missing something?!

@herestomwiththeweather

This comment has been minimized.

Collaborator

herestomwiththeweather commented Feb 15, 2013

scroll down a bit to january 11 commits by @rewritten https://github.com/oscurrency/oscurrency/commits/rails329

@acao

This comment has been minimized.

acao commented Feb 15, 2013

Oops! Someone working on our project had switched to the master branch and didn't tell me. Nevermind. Looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment