Clarify documentation on this.on(selector, ...)

[staeke]

Currently the documentation for this.on reads

Optional. Specify the DOM node(s) that should listen for the event. Defaults to the component instance's node value.

I've made the mistake to believe that the selector (if it's a string) is interpreted as relative to this.$node. Please make it clear that it's a global selector.

[patricknixon]

+1

I made the same assumption. It might be a good idea to also show how to use relative selectors:

this.on(this.select('selectorAttribute'), eventType, handler)

OR

this.on(eventType, { selectorAttribute: handler })

It should be noted somewhere that in the first case, event handlers are attached directly to elements matching the selector, and are not delegated to the current node like they would be if one passes an object as the second argument

// this handler is attached directly to the matching element:
this.on(selector, eventType, handler)

// this handler is attached to this.node and and will capture events from
// all children that match `selector` now and in the future
this.on(eventType, { selector: handler })

[necolas]

I think the docs are quite clear on this though.

The signature clearly shows that the first arg is optional, can be any DOM node, and defaults to the root node. The first example shows an event being bound to document. It's also noted that the handler argument can be a map of targets to callbacks, and that this is effectively event delegation to the root node.

[staeke]

@necolas Hmm...I really don't agree. First, there hasn't been any confusion around whether the first arg is optional or whether one is able to send any DOM node, or that the default is this.node unless you pass a selector. That is clear. Also, there's no confusion around the case where you pass a map of targets.

The confusion is where you pass a string. Nowhere in the docs does it say that this selector is relative to document. Both @patricknixon and I assumed this.node. I believe others have/will as well.

My recommendation would be to, under "selector: String | Element | Element collection", include
"If selector is a string, that selector will be resolved from document. In other words, it would be equivalent to passing $(selector)"

[tbrd]

I'm not sure I agree either. I certainly made this mistake when I first started using Flight. It's easy to assume this behaviour, so some additional clarity around the actual behaviour would make sense.

[necolas]

If you read @patricknixon's comment, you'll see why I mentioned those other things.

I can't see why you'd assume that the selector is relative to this.node when that isn't said or implied anywhere, and the examples often use document as the first arg.

[tbrd]

To say "I can't see why you'd assume" rather than "I'm not sure I
understand. Can you tell me why you think..." feels rather aggressive. Can
we have a conversation, rather than an argument?

I shouldn't have to defend why I felt confused. That implies some stupidity
on my part. The very fact that myself and others were confused means that
this is in some way confusing.

On 23 July 2014 18:30, Nicolas Gallagher [email protected] wrote:

If you read @patricknixon https://github.com/patricknixon's comment,
you'll see why I mentioned those other things.

I can't see why you'd assume that the select is relative to this.node
when that isn't said or implied anywhere, and the examples often use
document as the first arg.

—
Reply to this email directly or view it on GitHub
#265 (comment).

[necolas]

OK. Going to reopen. Please improve this part of the docs as you see fit.

[staeke]

Thanks @tbrd for your comment and @necolas for reopening.

Regarding your question @necolas - about why I (and others) assume that it was relative to this.node - why assume that? Well, it seems weird for a component to know about the "other world" DOM structure. Or to have requirements on clients on how the outer document should be laid out - then it's not very well encapsulated IMO. So that seems like an odd use case. Secondly, having internal selectors (the click handler for that button rendered in our template) seems like a very common use case, that's why I assumed the overload was built for that purpose.

Another reason is that the signature VERY much looks looks like jQuery's on, which uses current node relative selectors.

[giuseppeg]

The jQuery's on signature is slightly different, and it would be worth mentioning that Flight's api != jQuery's one.
Because of the similarities people don't read the doc and get confused imho (I did the same mistake at the beginning).

Often times event delegation is really what you want to do. Personally I've never bound to this.select('descendants').

@staeke is right when he says that a component shouldn't be able to query the DOM like that, and maybe it would be nice to allow it to select only a subset of it eg. window, document, html, head, body, link, meta, script etc. but that's OT

[necolas]

it seems weird for a component to know about the "other world" DOM structure.

Yeah I know what you mean. I think we only use that feature to bind to the document to listen to global events (e.g., data updates). But you could probably use an event emitter for that rather than the DOM, and then enforce encapsulation in the library.

[angus-c]

The more I use flight, the more I see the value in binding to the document. I agree binding to anything between the node and the document (or body) feels a bit icky - but I don't see the point in being overly prescriptive.

[angus-c]

If you use this.select you are always binding relative to the node, and this forces you to use the selectors defined by the attrs (hence DRY). If you go by this principle (never hard code a selector outside the attrs) then the only elements you can bind to outside the component node are document and body - both of which are reasonable bind targets.

[staeke]

@angus-c This definitely seems like the recommended approach. The downside I see with that though is that internal selectors are overrideable by outside users of your component. You might want to lock those down to prevent that, or avoid the risk of having naming conflicts with other mixins.

[angus-c]

@staeke can you clarify what you mean by "outside users of your component"?. Since attrs are declared/assigned defaults within the component, they can only be overriden by either an attachTo argument or by a mixin (using the new attributes syntax). In both cases this overriding is probably by design.

[staeke]

@angus-c sure...guess I was a little unclear. I was primarily thinking of code that calls attachTo. By moving the selectors to attr you open of for the possibility of overriding, which is of course also valid in the case of a mixin. This may or may not be what you want. Point is, you don't have an option. There is not such thing as defaultPrivateAttrs or defaultProtectedAttrs.

[tbrd]

I think this conversation is getting off topic. The basic thing is, it
makes sense to have a line in the documentation that clarifies that this.on
does not behave like this.$node.on

On 24 July 2014 18:54, Staffan Eketorp [email protected] wrote:

@angus-c https://github.com/angus-c sure...guess I was a little
unclear. I was primarily thinking of code that calls attachTo. By moving
the selectors to attr you open of for the possibility of overriding,
which is of course also valid in the case of a mixin. This may or may not
be what you want. Point is, you don't have an option. There is not such
thing as defaultPrivateAttrs or defaultProtectedAttrs.

—
Reply to this email directly or view it on GitHub
#265 (comment).

[tgvashworth]

Pull request to fix this gladly accepted! @staeke

[tgvashworth]

Leaving this open but marking for 2.0.0 milestone, which will include a documentation revamp.