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

Proposal: adopt the on() standard #55

Closed
rvagg opened this issue Jan 25, 2012 · 8 comments
Closed

Proposal: adopt the on() standard #55

rvagg opened this issue Jan 25, 2012 · 8 comments
Milestone

Comments

@rvagg
Copy link
Collaborator

rvagg commented Jan 25, 2012

This is a request for comments on a bit of API breakage, I'd love to get feedback from Bean users on this, particularly if you use on().

on(), as it currently exists in the Ender bridge, is incompatible with the standard set by Prototype and adopted by jQuery.

I think the emerging on() standard is a good one and we should change Bean to conform to it. At least in the Ender bridge and perhaps in Bean proper. I suspect that since most jQuery users who are using Ender are probably using bind() or the other traditional jQuery style methods and since Bean's on() is not directly compatible with jQuery's it may not be getting much use.

Prototype's on() signature is: .on(element, eventName[, selector], handlerFn)

jQuery's on() signature is: .on(events [, selector] [, data], handlerFn)

Unfortunately, ours is (in Ender form): .on([selector ,] events, handlerFn) (note the order of selector and events)

I propose that we change Bean to this signature: .on(events [, selector], handlerFn) with the possibility of adding the jQuery data parameter in the future (I imagine it would be quite helpful, but perhaps you should just curry your handler).

This is a modified integrate() in the bridge that makes the change there:

    , integrate = function (method, type) {
        var _args = type ? [type] : []
        return function () {
          for (var args, i = 0, l = this.length; i < l; i++) {
            args = [this[i]].concat(_args, Array.prototype.slice.call(arguments, 0))
            if (args.length == 4) {
              args.push($)
              if (method == 'on') { // 'on' puts event type first, selector second
                var s = args[1]
                args[1] = args[2]
                args[2] = s
              }
            }
            b[!arguments.length && method == 'add' && type ?
              'fire' : method == 'on' ?
                'add' : method
            ].apply(this, args)
          }
          return this
        }
      }

// on: integrate('on')

It's a bit long and I think I'd rather see on() added to Bean proper, perhaps even encouraged as the primary interface for adding. The signature there would look something like: .on(element, events [,selector], handlerFn [, selectorEngine]).

I'm not sure where this leaves .off() though, it's a funny API although it's consistent with having an .on() I guess and we already have it in the bridge. Prototype doesn't bother with on(), you still use .stopObserving().

Incidentally, Prototype returns a special handler object that has start() and stop() methods that you can use to turn on and off the listener, so you don't really need an off().

@ded
Copy link
Collaborator

ded commented Jan 25, 2012

not that we conform too much to jQuery's API — but I do like the good parts. i can't even say i like on(selector, events) — that seems backwards, so i'm for this change. i didn't originally design bean — but you should never have optional leading arguments.

@rvagg
Copy link
Collaborator Author

rvagg commented Jan 25, 2012

On conformance: it helps people switch from jQuery and it also makes developing jQuery & Ender compatible plugins/modules so much easier (like exposing $.fn).

I'm not sure about the other major libraries but the fact that jQuery has gone the Prototype path makes is making it a bit of a standard. Of course Zepto also has the same API (minus data parameter).

@iamdustan
Copy link

What’s the current status/thoughts on this change?

@rvagg
Copy link
Collaborator Author

rvagg commented Apr 11, 2012

Going for it! when I have some breathing space anyway. The current plan is to take the newfixevent branch, merge the latest changes (setSelectorEngine and IE8 support for the 'message' event), rename it to 0.5-wip and start making a mess! I put some notes in https://github.com/fat/bean/issues/milestones about things I'd like try and tackle. Priority is to keep size under control though so a lot of it is cleaning up messes. But doing a proper on() is certainly near the top of the list I think.

@activestylus
Copy link

+1

@ghost
Copy link

ghost commented Jun 11, 2012

Can use this too!

@rvagg
Copy link
Collaborator Author

rvagg commented Aug 18, 2012

Due for a 1.0 release, working code is in the 0.5-wip branch and you can try it out in an Ender build by installing bean@dev. Full details here: http://rod.vagg.org/2012/08/bean_v1/

@rvagg
Copy link
Collaborator Author

rvagg commented Sep 13, 2012

all done, 1.0 is out with this change

@rvagg rvagg closed this as completed Sep 13, 2012
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

No branches or pull requests

4 participants