Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Added a bunch of jQuery javascript helpers and more... #76

Open
wants to merge 33 commits into from

3 participants

@kristianmandrup

Made it more compatible with asset pipeline and improved generator placement structure

kristianmandrup added some commits
@kristianmandrup kristianmandrup some nice updates to make it better to work with for Rails 3.2 2799ba7
@kristianmandrup kristianmandrup all test pass 40acbeb
@kristianmandrup kristianmandrup added api docs 2e8b36d
@kristianmandrup kristianmandrup much cleaner and flexible 492dc03
@kristianmandrup kristianmandrup jquery helper tests wip cb80a00
@kristianmandrup kristianmandrup all tests pass using new jquery helpers 22263c5
@kristianmandrup kristianmandrup bump version 27c26bf
@kristianmandrup kristianmandrup added more view helpers for js 25e1d5f
@kristianmandrup kristianmandrup patch cell rendering to use views folder b17ec08
@kristianmandrup kristianmandrup all specs pass - much better coffe/js integration using js/coffee nam…
…espaces and classes for widgets :)
5432319
@kristianmandrup kristianmandrup better coffee namespace docs 2b3f2ef
@kristianmandrup kristianmandrup better widget client side infra ff6d7ca
@kristianmandrup kristianmandrup adjusted widget target path to fit with rails standards 95dc381
@kristianmandrup kristianmandrup using engine 706a353
@kristianmandrup kristianmandrup much better generator structure and flexibility - all tests pass 9a9b9cd
@kristianmandrup kristianmandrup allow stylesheet lang option on generator 1607f57
@kristianmandrup kristianmandrup fix js naming 9856a54
@kristianmandrup kristianmandrup fix rendering? a6d026e
@kristianmandrup kristianmandrup add engine setup generator 8b1a46e
@kristianmandrup kristianmandrup more README instructions 99d55e8
@kristianmandrup kristianmandrup added skeleton to test for widget with action named load - which curr…
…ently fails
c82e335
@kristianmandrup kristianmandrup support jQuery noConflict mode 78a2bc3
@kristianmandrup kristianmandrup jquery noConflic mode - tests passes 7b141be
@kristianmandrup kristianmandrup global partials 1ee5fe9
@kristianmandrup kristianmandrup add essential cells documentation to apotomo README 5e8abdc
@kristianmandrup kristianmandrup added kaminari setup generator 41f56ed
@kristianmandrup kristianmandrup notes on url helpers b70e4d6
@kristianmandrup kristianmandrup using rspec with apotomo c300cc8
@kristianmandrup kristianmandrup uncommented travis for now 847df85
@kristianmandrup kristianmandrup fixed kaminari setup generator 518b824
@kristianmandrup kristianmandrup notes on use with rspec 02981ae
@kristianmandrup kristianmandrup how to configure generators 3b5e23b
@kristianmandrup kristianmandrup minor twist in readme 5431312
@apotonick
Owner

Can you please fix the tests, I get flooded with travis emails telling me the build is "still broken".

@kristianmandrup

All tests pass on my side locally with ruby 1.9.3. I don't know why it pushes to your Travis. I thought only you had access to do this? I don't have much experience with using Travis yet, sorry.
I even tried renaming the .travis.yml file, in order to avoid pushing it to Travis for now...
I'm mostly a "coder", not so experienced with all this administration/configuration/deployment etc.

@kristianmandrup

Nice! I will be using widgets in my current project big time. I will soon have some time to contribute more... I also need to understand how to use Travis correctly so as not to cause these floods of errors in the future.

@kuraga

@kristianmandrup it's very nice! Don't you remember why test is fail? I'll explain in future because this bunch is very cool and useful!

P.S. Maybe it's a reason to see #22 also. For other JavaScript helpers...

@kristianmandrup

@kuraga, let me add you on Skype, then we can discuss more how to proceed...

@kuraga

@kristianmandrup There is a problem in commit kristianmandrup/apotomo@c82e335. What is it for?

Maybe see #48.

Failure:

1) Failure: test: A widget should list 'load' in local and inherited states in Widget.action_methods. (WidgetTest) [apotomo/test/widget_test.rb:138]: Failed assertion, no message given.
169 tests, 293 assertions, 1 failures, 0 errors, 0 skips

The test:

    should "list 'load' in local and inherited states in Widget.action_methods" do
      assert MouseWidget.action_methods.collect{ |m| m.to_s }.include?("load")
      assert Class.new(MouseWidget).action_methods.collect{ |m| m.to_s }.include?("load")
    end
@kristianmandrup

I have no idea. I remember as well that I was puzzled over this test as I ran into it…

It just means that the method "load" should be a member of the action_methods array, hmm… not sure what load is used for. I guess it "loads" the widget and its state? Check the code. I'm too busy, sorry!

@apotonick
Owner
@kuraga

Small style code fixes are needed here and later... I.e. Convert def meth var to def meth(var). And don't you prefer each to for?

@kuraga

Why not to unify quotes politics?
element(id) + ".find(#{selector}).append('#{escape(markup)}');" (is)
"#{element(id)}.find(#{selector}).append('#{escape(markup)}');"
%Q<#{element(id)}.find(#{selector}).append('#{escape(markup)}');>

@kuraga

Without slashes maybe (not changed later)?

@kuraga

Have we sense to move this change to Cells?

It was a patch specifically to override the default behavior as defined in cells so that the views are not mixed with the action classes, but can live in a separate views folder "next to" that action class. Another option would be to have a separate views folder at the widgets (root) level like in Rails. Simpy having a views folder with the action class however would make it easier to move refactor and move around the action class with views as one entity. The other (more Rails like) approach would require more "synchronization effort".

I mean that this option (in some variant) is good for Cells too... And is it good when Cells and Apotomo behavior is equivalent, isn't it?

@kuraga

I don't think that it's right to show rspec situation... Because rspec usage is a custom situation and dependences on rspec-apotomo gem? Yes?
P.S. Maybe haml too... But it's easy to use haml syntax in examples and Nick always did that... :)

@kuraga

Attention!!! It seems that incompatibilities have been introduced here!

@kuraga

Ok, I have seen half of commits. And I saw two incompatibilities: views are moved to views subfolder and display default state is renamed to show... Your opinion, Nick?
UPD: It seems that situation of an old behavior are handled, too.

@kuraga

Hmm... I don't understand apotomo's log system at all :) puts but don't puts... Log rendering a view... but don't log which of them. Don't you need rails's log system integration? I can help maybe.

@kuraga

capture doesn't demand a proxy (I mean b variable here). Can't we realize that here?

@kuraga

Sorry but can't we use something from actionpack to realize it?..

@kuraga

What is this commit for? It breaks tests maybe because load is a Kernel method. Revert it!

@kristianmandrup

This was to make it easier to implement Ajax dragn/drop via jQuery Ajax helpers, as demonstrated in one of the apotomo demos.
I never got around to test it for real. Feel free to make your changes as you see fit.

@apotonick
Owner

Could we have separate discussions about the (upcoming?) views/ subfolder for cells and widgets and the jQuery helpers?

@kuraga

Strange section... Is this sections for Cell, no? And see #21 and this comment.

@kuraga

Nick, what do you think about this method?

@kuraga

Would we create code (that doesn't fix issues) for very special gems, Nick?

@kuraga

Why the.travis.yml?

@kuraga

Yes, sure. Do you mean new github issue?

@kuraga

@kristianmandrup Don't you remember some (else) incompatibilities introduced by this PR?

@apotonick
Owner

It would be great if this PR gets split into features that can be merged separately as I don't have the time to go through all the commits. We don't need to open more PRs, just a mapping from commits to feature.

@kuraga

No. These things are not separated by Nick (i.e. see the first commit). By think that I can split each diff... And squash up then. But I need time for this.

@kristianmandrup

To me it makes perfect sense for the default action to be named #show to be in line with Rails REST conventions.
Perhaps there should be an option to somehow indicate that a widget can handle both a collection and simple items via #index and #show ? Do widgets even need views in the form of templates in the traditional Rails sense?

One could argue that the best practice would be to partition the interface into sufficiently small parts, that each "view" could better be handled by a Decorator or similar and take advantage of inheritance etc for better reuse and encapsulation of business logic? Just some thoughts... I would think, that ideally the templates should only be used as Proof of Concept, before refactoring them into decorators? At least, that would be my choice.

@apotonick
Owner

@kuraga Wait, first I need an overview about which features are in the PR. I can see jQuery extensions right away.

@kristianmandrup The default state of a widget should be #show as #display is an internal ruby method. Also, I agree, it matches with Rails' CRUD convention.

What I don't understand is that decorator thing? Do you mean to decorate the widget, or the models contained in the widget?

@kristianmandrup

What I mean is that if you have a widget, which encapsulates a small view part, it can become a mess with all these mini view templates IMO. I feel that a better approach is to have the view rendered directly via a Decorator class, which you can test independently, something like Draper for example. Then it could look sth like:

render text: decorator.show(self)

I would imagine?

class MyWidget < Widget
  def show
    render text: decorator.show(self)
  end

  protected

  def decorator widget
    MyWidgetDecorator.new(widget).show
  end
end

A "CRUDe" example ;) but the basic idea... The Decorator then gets all the power of OOP, such as inheritance, polymorphism, modularization etc. I think it is a small price to pay for the little extra complexity. The view templates could of course still be used in the quick Proof of Concept (prototype) phase. Just my thoughts...

@kuraga

People let's separate discussions! The age of this PR is half-of-year. We should step forward...

@apotonick
Owner
@kuraga

Ou!!!!!!!! Name error!!!!

@kuraga

Why callback-version method is defined for toggle variant only?
And why doesn't callback receive any arguments?

@kristianmandrup

I didn't test all these helpers. Just started adding whichever I thought might be useful... not even sure this is the right approach :P Perhaps there is a better way, without having ruby "spit out" javascript? Perhaps better with more clever use of Object Oriented coffeescript? Maybe have a few predefined coffeescript functions that handle this instead?

@kuraga

@kristianmandrup example please?

@kristianmandrup

Imagine we have an Apotomo.Widget object in coffeescript:

class Apotomo.Widget
  initialize(id) -> 
    @id = id

  execute(method, *args)  ->
    # dynamically execute jquery function on widget (via @id)

  # or just have each function like this...
  toggleClass(name ->
    $(id).toggle(name)

Putting the functionality where it "belongs" ;) no?

@kuraga

@kristianmandrup Do you say about functionality which is in commits now? If yes then I think that is good have both functionalities... Your example is very nice but what if I don't want create some special JS objects?...

But about helpers... Should they be JS-agnostic maybe? Without jQuery?

@kristianmandrup

I would add jQuery helpers, since this is the new default as I understand since Rails 3.1. If some developers want to add equivalent helpers for other javascript frameworks or pure javascript, they may do so... leave room for others ;)
Should just be "proof of concept" to target most common usage, others may expand this if they need.

@kuraga

Yes, I agree. Oh, I see! Frameworks are separated by modules.

@kuraga

Ok, @apotonick @kristianmandrup I think I should split these commits and analyze them. I will do this. For own experience or for Apotomo or for my project, I don't care... I can say nothing about time... And sure you can do this work parallel with/out me. Thanks.

@kristianmandrup

Look at this :)

class Apotomo.WidgetRepo
  initialize ->
    @widgets = []

  find(id) ->
    @widgets[id]

  register(widget) ->
    @widgets[widget.id] = widget

Apotomo.widgets = new Apotomo.WidgetRepo

class Apotomo.Widget
  initialize id ->
    @id = id
    Apotomo.widgets.register(self) # or this ?

  element ->
    $(id)

  toggleClass(name) -> 
    element.toggleClass(name)

  hide ->
    element.hide

class Property.TabWidget extends Apotomo.Widget
  initialize(id) ->
    super(id)

  show ->
    # ...

  collapse ->

  expand ->

PS: Above syntax may not be correct, but should make it clear what I mean

in ruby Widget class render method

class Property::TabWidget < Apotomo::Widget

  def render
    # some ruby code... fx operations on instance vars

    # some ruby expression to render the following in javascript
    # Apotomo.widgets.find(id).toggleClass('highlight`)

     # for example
     js_for(widget_id) do |widget|
       widget.toggleClass(highlight)
     end
  end

I think this approach would be way better, and nice encapsulation of functionality at both ends.

@kuraga

Yes, sure, but I think it's not bad to have append method if we have replace method! Which we do have now; or do you suggest to remove JavascriptGenerators module at all?

@kristianmandrup

I suggest to replace the ruby side with something like this:

My own widget

     client_widget.toggleClass(highlight)

Operate on some other widget by id

     client_widget(widget_id) do |widget|
       widget.toggleClass(highlight)
     end

or operate on multiple widgets

     client_widgets(id1, id2) do |widgets|
       widgets.each {|widget| widget.toggleClass(highlight) }
     end

In each case, rendering javascript like this (to retrieve the Javascript widget class instance from the Javascript widget repository)

Apotomo.widgets.find(id)

Nice and elegant :)

The JavaScriptGenerator then have to be refactored to support this new style... that's all.

@kuraga

Hm... I thought that you suggest to introduce execute helper (with some code as argument) and remove other helpers...

Yes! I understand now! Very cool! @apotonick, Nick, your opinion?

P.S. Maybe nice to delegate toggleClass to client_widget automatically :)

@kristianmandrup

Well, execute was meant as a more generic helper.

widget.execute(:highlight, 'orange') would render javascript such as:

widget.execute('highlight', orange') which would execute something like the following (coffeescript):

  execute(method, *args) ->
    $(id)[method](*args)

I think execute could be useful in addition to the other helpers (at least for prototyping functionality), but preferably the developer should populate the Javascript "twin class" for each widget with whatever specific methods it needs, and not rely on such a generic function. What do you think?

P.S: I don't understand what you mean by "delegate toggleClass to client_widget automatically" ? toggleClass was just as an example.

@kuraga

I think that my experience in thinking of patterns is too small for this :) Let's wait for Nick's opinion.

One that I think - I think that helpers should be or not be. Or a comprehensive set of them, or one helper render only.

P.S. toggleClass(:highlight) may be equivalent to client_widget.toggleClass(:highligt).

@kristianmandrup

ahh, but that is dangerous in my opinion. It is not clear in that case that toggleClass is applied on the client side widget and rendered as javascript. Too much magic! Why I introduced the block syntax, in order to make it clear that whatever was inside the block scope is rendered as javascript ;)

@kuraga

Yeah, yeah... That's a pattern too. Will you, @kristianmandrup, code something two nearest weeks?

@kristianmandrup

Yeah, if you get started by taking the best of my ideas and cleaning up the code, I will be ready to help out from next week or this weekend ;)

@kuraga

I'll start by splitting commits only (and reading them). Then'll talk ;)

@kristianmandrup

Skype!?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 5, 2012
  1. @kristianmandrup
  2. @kristianmandrup

    all test pass

    kristianmandrup authored
  3. @kristianmandrup

    added api docs

    kristianmandrup authored
  4. @kristianmandrup
  5. @kristianmandrup
  6. @kristianmandrup
  7. @kristianmandrup

    bump version

    kristianmandrup authored
Commits on Sep 6, 2012
  1. @kristianmandrup
  2. @kristianmandrup
  3. @kristianmandrup

    all specs pass - much better coffe/js integration using js/coffee nam…

    kristianmandrup authored
    …espaces and classes for widgets :)
  4. @kristianmandrup
  5. @kristianmandrup
Commits on Sep 7, 2012
  1. @kristianmandrup
  2. @kristianmandrup

    using engine

    kristianmandrup authored
Commits on Sep 10, 2012
  1. @kristianmandrup
  2. @kristianmandrup
  3. @kristianmandrup

    fix js naming

    kristianmandrup authored
Commits on Nov 22, 2012
  1. @kristianmandrup

    fix rendering?

    kristianmandrup authored
  2. @kristianmandrup
  3. @kristianmandrup
  4. @kristianmandrup
  5. @kristianmandrup
  6. @kristianmandrup
  7. @kristianmandrup

    global partials

    kristianmandrup authored
  8. @kristianmandrup
  9. @kristianmandrup
  10. @kristianmandrup

    notes on url helpers

    kristianmandrup authored
  11. @kristianmandrup
  12. @kristianmandrup
  13. @kristianmandrup
  14. @kristianmandrup
  15. @kristianmandrup
  16. @kristianmandrup

    minor twist in readme

    kristianmandrup authored
Something went wrong with that request. Please try again.