lml / ost

OpenStax Tutor
Other
7 stars 8 forks source link

lml/ost#347: Integrate unconfirm gem. #352

Closed navilan closed 10 years ago

navilan commented 10 years ago
navilan commented 10 years ago

@Dantemss / @jpslav : This includes the review of: https://github.com/lml/unconfirm. Things that need attention:

  1. This is more than what I anticipated to be the number of steps for integrating the gem. But, I don't see a way around it. Any suggestions to cut down those 5 steps? https://github.com/lml/unconfirm#usage
  2. There are a couple of places where I had to explicitly include helpers. I read mixed information about why this doesn't work as expected. The summary being, this is not needed in later versions of rails. Is there a way to remove those explicit includes? https://github.com/lml/unconfirm/blob/master/app/controllers/unconfirm/application_controller.rb#L3 https://github.com/lml/unconfirm/blob/master/app/controllers/unconfirm/user_settings_controller.rb#L3
  3. I was expecting that with this: https://github.com/lml/unconfirm/blob/master/lib/unconfirm/engine.rb#L5 I didn't need to worry about the JS files. But even with that an explicit //= require unconfirm is required.

TODO:

Dantemss commented 10 years ago

For step 2, you could combine these 2 methods into 1.

Actually, you could probably combine 2 and 5 into 1 method (it doesn't matter if the stuff in 2 is in the body, right? If necessary, you could even set some template variables to prevent the stuff in 2 from being included twice in the page).

If you don't mind not using the asset pipeline, you could also put the javascript in 3 inline with 2 and 5. This would, however, mean the page would load marginally slower, as the browser would not cache the javascript. But it would probably be negligible.

Personally I would probably combine 2 and 5, but keep 3 separate.

navilan commented 10 years ago

@Dantemss - Thanks.

Yeah, for step2 - I kept the methods separate as one inserts html and the other one JS. I could perhaps provide a third method that injects both together.

yeah - I'd really like the loading to be separate from the script tags. It does make the page a lot slower.

Do you know of a better way to deal with the helpers?

Dantemss commented 10 years ago

I prefer to avoid helpers entirely and handle helper methods like this:

https://github.com/openstax/action_interceptor/blob/master/lib/action_interceptor/view.rb

I only have an include Common in there, but you can add any instance method and it will work in all views.

Dantemss commented 10 years ago

If you need something from the controller object and/or if the method should also be available in controllers, I do it like this:

https://github.com/openstax/action_interceptor/blob/master/lib/action_interceptor/controller.rb

The only reason I needed both in action_interceptor is that, unfortunately, url_for works differently in controllers vs views (Rails "magic").