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

A little clarification. #19

Merged
merged 1 commit into from
Mar 14, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions backbone.js
Original file line number Diff line number Diff line change
Expand Up @@ -1065,15 +1065,15 @@
// Remove this view by taking the element out of the DOM, and removing any
// applicable Backbone.Events listeners.
remove: function() {
this._remove();
this._removeElement();
this.stopListening();
return this;
},

// 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() {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some grepping and _remove is a fairly popular method name in views already. Also, I think _removeElement is a bit more accurate.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

K good call. I think it was removeElement first, then we were going for parity between the setElement/remove and internal methods.

this.$el.remove();
},

Expand Down Expand Up @@ -1117,10 +1117,8 @@
var method = events[key];
if (!_.isFunction(method)) method = this[events[key]];
if (!method) continue;

var match = key.match(delegateEventSplitter);
var eventName = match[1], selector = match[2];
this.delegate(eventName, selector, _.bind(method, this));
this.delegate(match[1], match[2], _.bind(method, this));
}
return this;
},
Expand Down
76 changes: 24 additions & 52 deletions test/view.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,13 @@
ok(result.length === +result.length);
});

test("jQuery", function() {
test("$el", 3, function() {
var view = new Backbone.View;
view.setElement('<p><a><b>test</b></a></p>');

strictEqual(view.el.nodeType, 1);

if (Backbone.$) {
ok(view.$el instanceof Backbone.$);
strictEqual(view.$el[0], view.el);
} else {
strictEqual(view.$el, undefined);
}
ok(view.$el instanceof Backbone.$);
strictEqual(view.$el[0], view.el);
});

test("initialize", 1, function() {
Expand Down Expand Up @@ -78,18 +73,14 @@
});

test("delegate", 2, function() {
var counter1 = 0, counter2 = 0;

var view = new Backbone.View({el: '#testElement'});
view.increment = function(){ counter1++; };
view.$el.on('click', function(){ counter2++; });

view.delegate('click', 'h1', view.increment);
view.delegate('click', view.increment);

view.delegate('click', 'h1', function() {
ok(true);
});
view.delegate('click', function() {
ok(true);
});
view.$('h1').trigger('click');
equal(counter1, 2);
equal(counter2, 1);
});

test("delegateEvents allows functions for callbacks", 3, function() {
Expand Down Expand Up @@ -167,33 +158,22 @@
});

test("undelegate with selector", 2, function() {
var counter1 = 0, counter2 = 0;
view = new Backbone.View({el: '#testElement'});
view.delegate('click', function() { counter1++; });
view.delegate('click', 'h1', function() { counter2++; });

view.delegate('click', function() { ok(true); });
view.delegate('click', 'h1', function() { ok(false); });
view.undelegate('click', 'h1');

view.$('h1').trigger('click');
view.$el.trigger('click');

equal(counter1, 2);
equal(counter2, 0);
});

test("undelegate with handler and selector", 2, function() {
var counter1 = 0, counter2 = 0;
view = new Backbone.View({el: '#testElement'});
view.delegate('click', function() { counter1++; });
var handler = view.delegate('click', 'h1', function() { counter2++; });

view.delegate('click', function() { ok(true); });
var handler = function(){ ok(false); };
view.delegate('click', 'h1', handler);
view.undelegate('click', 'h1', handler);

view.$('h1').trigger('click');
view.$el.trigger('click');

equal(counter1, 2);
equal(counter2, 0);
});

test("_ensureElement with DOM node el", 1, function() {
Expand Down Expand Up @@ -284,25 +264,18 @@
});

test("custom events", 2, function() {
var count = 0;

var View = Backbone.View.extend({
el: $('body'),
events: function() {
return {"fake$event": "run"};
},
run: function() {
count++;
events: {
"fake$event": function() { ok(true); }
}
});

var view = new View;
$('body').trigger('fake$event').trigger('fake$event');
equal(count, 2);

$('body').off('fake$event');
$('body').trigger('fake$event');
equal(count, 2);
});

test("#1048 - setElement uses provided object.", 2, function() {
Expand Down Expand Up @@ -353,7 +326,7 @@
}
});

ok(new View().el.tagName.toLowerCase() == 'p');
ok(new View().$el.is('p'));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is maybe fine but let's deal with de-jQuerying tests separately if need be.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

naw. we've been trying to de-jqueryify tests for a while (at least I have). this is perfectly fine as a tagName check.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but it's just noise in jashkenas#3003 where there's already enough. Let's split it out!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

urgh ;) fine.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize I'm saying this in a pull where I've included too much noise. I'll try to fix that. :)

});

test("views stopListening", 0, function() {
Expand Down Expand Up @@ -382,8 +355,8 @@
});

var view = new View;
ok(view.el.tagName.toLowerCase() == 'p');
ok(view.$('a').length != 0);
ok(view.$el.is('p'));
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here too.

ok(view.$el.has('a'));
});

test("events passed in options", 1, function() {
Expand All @@ -406,20 +379,19 @@
equal(counter, 2);
});

test("remove", 2, function() {
var el = view.el;
test("remove", 1, function() {
var view = new Backbone.View;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how come? Isn't the setup view good enough?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair 'nuff. But noise and all that?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha! This is a brand new test though, right?

document.body.appendChild(view.el);

view.delegate('click', function() { ok(false); });
view.listenTo(view, 'all x', function() { ok(false); });

view.remove();

strictEqual(view.el, el);
notEqual(view.el.parentNode, document.body);

view.$el.trigger('click');
view.trigger('x');

// In IE8 and below, parentNode still exists but is not document.body.
notEqual(view.el.parentNode, document.body);
});

})();