A little clarification. #19
Conversation
| @@ -1073,15 +1073,15 @@ | |||
| // Remove this view's element from the document and all event listeners | |||
| // attached to it. Exposed for subclasses using an alternative DOM | |||
| // manipulation API. | |||
| _remove: function() { | |||
| _removeElement: function() { | |||
There was a problem hiding this comment.
I did some grepping and _remove is a fairly popular method name in views already. Also, I think _removeElement is a bit more accurate.
There was a problem hiding this comment.
K good call. I think it was removeElement first, then we were going for parity between the setElement/remove and internal methods.
| @@ -1149,6 +1142,7 @@ | |||
| // `selector` and `listener` are both optional. | |||
| undelegate: function(eventName, selector, listener) { | |||
| this.$el.off(eventName + '.delegateEvents' + this.cid, selector, listener); | |||
| return this; | |||
There was a problem hiding this comment.
Really not sure about this. Now you can do this:
view.undelegate('click', view.onClick).delegateEvents() // oops redelegated againThere was a problem hiding this comment.
I think you could just as easily call the two in succession. I'm not overly worried about this though. I'll drop the return.
|
Since we are doing cleanups anyway, can we refactor away this constant |
|
@wyueho its not a constant. It's unique per-view. And setting it as a property on the view when its only used twice seems overkill. |
|
I meant drying it up :) Sent from my iPhone
|
|
What do you propose? |
| strictEqual(view.el.nodeType, 1); | ||
|
|
||
| if (Backbone.$) { | ||
| ok(view.$el instanceof Backbone.$); | ||
| strictEqual(view.$el[0], view.el); | ||
| } else { | ||
| strictEqual(view.$el, undefined); |
There was a problem hiding this comment.
We already decided elsewhere to leave testing subclasses with Backbone tests out of jashkenas#3003. Just sticking to that.
There was a problem hiding this comment.
Ok cool. This is with the assumption that if you're using View you've got Backbone.$ set, yeah? We can tease out the acceptance tests later then probably.
There was a problem hiding this comment.
Maybe pull out the Backbone.$ check then?
|
@akre54 Updated! Anything else? |
| @@ -29,18 +29,13 @@ | |||
| ok(result.length === +result.length); | |||
| }); | |||
|
|
|||
| test("jQuery", function() { | |||
| test("$el", function() { | |||
* Rename #_remove as #_removeElement. * Add #_setAttributes instead of using #setElement. * Slim down tests a bit. * #delegate returns this.
| test("remove", 2, function() { | ||
| var el = view.el; | ||
| test("remove", 1, function() { | ||
| var view = new Backbone.View; |
There was a problem hiding this comment.
how come? Isn't the setup view good enough?
There was a problem hiding this comment.
Been trying for a long time to get away from the setup view/collection/model/router/history. Using shared objects tends to cause subtle bugs or change the meaning of other tests unintentionally, especially when it's just as concise to use an isolated instance.
There was a problem hiding this comment.
Fair 'nuff. But noise and all that?
There was a problem hiding this comment.
Ha! This is a brand new test though, right?
This resolves my remaining issues with view hooks and I thought it would be easier to discuss here than on the ever growing hooks thread. Comments inline. :)
#_removeas#_removeElement.#_setAttributesinstead of using#setElement.#delegatereturns this.