Skip to content

Commit

Permalink
[CLEANUP] reducing private API surface of views.
Browse files Browse the repository at this point in the history
* remove unused internal `_willDestroyElement` hook
* cleanup `childViews` documentation not to refer to legacy methods
* remove undocumented `forEachChildView` used only by visibility support
  and refactor visibility support not to use it.
* remove `remove` and `removeFromParent` methods, refactor legacy style
  tests to avoid these.  Removing a view should be done via the template
  layer. Still leaves destroyElement which is equivalent and used in more
  legacy style tests.
  • Loading branch information
krisselden committed Jun 29, 2016
1 parent 183f646 commit de3bc9a
Show file tree
Hide file tree
Showing 9 changed files with 25 additions and 132 deletions.
5 changes: 1 addition & 4 deletions packages/ember-htmlbars/lib/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -268,9 +268,6 @@ Renderer.prototype.renderElementRemoval =
Renderer.prototype.willRemoveElement = function (/*view*/) {};

Renderer.prototype.willDestroyElement = function (view) {
if (view._willDestroyElement) {
view._willDestroyElement();
}
if (view.trigger) {
view.trigger('willDestroyElement');
view.trigger('willClearRender');
Expand All @@ -295,7 +292,7 @@ Renderer.prototype.didDestroyElement = function (view) {
if (view.trigger) {
view.trigger('didDestroyElement');
}
}; // Element destroyed so view.destroy shouldn't try to remove it removedFromDOM
};

export const InertRenderer = {
create({ dom }) {
Expand Down
1 change: 0 additions & 1 deletion packages/ember-views/lib/mixins/child_views_support.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ export default Mixin.create({

/**
Array of child views. You should never edit this array directly.
Instead, use `appendChild` and `removeFromParent`.
@property childViews
@type Array
Expand Down
62 changes: 6 additions & 56 deletions packages/ember-views/lib/mixins/view_support.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { assert, deprecate } from 'ember-metal/debug';
import { get } from 'ember-metal/property_get';
import run from 'ember-metal/run_loop';
import { guidFor } from 'ember-metal/utils';
import { Mixin } from 'ember-metal/mixin';
Expand Down Expand Up @@ -27,17 +26,18 @@ export default Mixin.create({
@param {Class,Mixin} klass Subclass of Ember.View (or Ember.View itself),
or an instance of Ember.Mixin.
@return Ember.View
@deprecated use `yield` and contextual components for composition instead.
@private
*/
nearestOfType(klass) {
let view = get(this, 'parentView');
let view = this.parentView;
let isOfType = klass instanceof Mixin ?
view => klass.detect(view) :
view => klass.detect(view.constructor);

while (view) {
if (isOfType(view)) { return view; }
view = get(view, 'parentView');
view = view.parentView;
}
},

Expand All @@ -47,14 +47,16 @@ export default Mixin.create({
@method nearestWithProperty
@param {String} property A property name
@return Ember.View
@deprecated use `yield` and contextual components for composition instead.
@private
*/
nearestWithProperty(property) {
let view = get(this, 'parentView');
let view = this.parentView;

while (view) {
if (property in view) { return view; }
view = get(view, 'parentView');
view = view.parentView;
}
},

Expand Down Expand Up @@ -110,21 +112,6 @@ export default Mixin.create({
return this._currentState.$(this, sel);
},

forEachChildView(callback) {
let childViews = this.childViews;

if (!childViews) { return this; }

let view, idx;

for (idx = 0; idx < childViews.length; idx++) {
view = childViews[idx];
callback(view);
}

return this;
},

/**
Appends the view's element to the specified parent element.
Expand Down Expand Up @@ -264,25 +251,6 @@ export default Mixin.create({
return this.appendTo(document.body);
},

/**
Removes the view's element from the element to which it is attached.
@method remove
@return {Ember.View} receiver
@private
*/
remove() {
// What we should really do here is wait until the end of the run loop
// to determine if the element has been re-appended to a different
// element.
// In the interim, we will just re-render if that happens. It is more
// important than elements get garbage collected.
if (!this.removedFromDOM) { this.destroyElement(); }

// Set flag to avoid future renders
this._willInsert = false;
},

/**
The HTML `id` of the view's element in the DOM. You can provide this
value yourself but it must be unique (just as in HTML):
Expand Down Expand Up @@ -557,24 +525,6 @@ export default Mixin.create({
}
},

/**
Removes the view from its `parentView`, if one is found. Otherwise
does nothing.
@method removeFromParent
@return {Ember.View} receiver
@private
*/
removeFromParent() {
let parent = this.parentView;

// Remove DOM element from parent
this.remove();

if (parent) { parent.removeChild(this); }
return this;
},

/**
You must call `destroy` on a view to destroy the view (and all of its
child views). This will remove the view from any parent node, then make
Expand Down
22 changes: 10 additions & 12 deletions packages/ember-views/lib/mixins/visibility_support.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,36 +67,34 @@ export default Mixin.create({

_notifyBecameVisible() {
this.trigger('becameVisible');

this.forEachChildView(view => {
let childViews = this.childViews;
for (let i = 0; i < childViews.length; i++) {
let view = childViews[i];
let isVisible = get(view, 'isVisible');

if (isVisible || isVisible === null) {
view._notifyBecameVisible();
}
});
}
},

_notifyBecameHidden() {
this.trigger('becameHidden');
this.forEachChildView(view => {
let childViews = this.childViews;
for (let i = 0; i < childViews.length; i++) {
let view = childViews[i];
let isVisible = get(view, 'isVisible');

if (isVisible || isVisible === null) {
view._notifyBecameHidden();
}
});
}
},

_isAncestorHidden() {
let parent = get(this, 'parentView');

let parent = this.parentView;
while (parent) {
if (get(parent, 'isVisible') === false) { return true; }

parent = get(parent, 'parentView');
parent = parent.parentView;
}

return false;
}
});
4 changes: 2 additions & 2 deletions packages/ember-views/tests/views/checkbox_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,12 +110,12 @@ QUnit.test('checked property mirrors input value', function() {

equal(checkboxComponent.$().prop('checked'), true, 'changing the value property changes the DOM');

run(() => checkboxComponent.remove());
run(() => checkboxComponent.destroyElement());
run(() => checkboxComponent.append());

equal(checkboxComponent.$().prop('checked'), true, 'changing the value property changes the DOM');

run(() => checkboxComponent.remove());
run(() => checkboxComponent.destroyElement());
run(() => set(checkboxComponent, 'checked', false));
run(() => checkboxComponent.append());

Expand Down
2 changes: 1 addition & 1 deletion packages/ember-views/tests/views/view/append_to_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ QUnit.test('remove removes an element from the DOM', function() {

ok(jQuery('#' + get(view, 'elementId')).length === 1, 'precond - element was inserted');

run(() => view.remove());
run(() => view.destroyElement());

ok(jQuery('#' + get(view, 'elementId')).length === 0, 'remove removes an element from the DOM');
ok(EmberView.views[get(view, 'elementId')] === undefined, 'remove does not remove the view from the view hash');
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ QUnit.skip('classNameBindings lifecycle test', function() {
equal(isWatching(view, 'priority'), true);

run(() => {
view.remove();
view.destroyElement();
view.set('priority', 'low');
});

Expand Down
8 changes: 4 additions & 4 deletions packages/ember-views/tests/views/view/is_visible_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ QUnit.test('should hide views when isVisible is false', function() {
run(() => set(view, 'isVisible', true));

ok(view.$().is(':visible'), 'the view is visible');
run(() => view.remove());
run(() => view.destroyElement());

deepEqual(warnings, [], 'no warnings were triggered');
});
Expand All @@ -57,15 +57,15 @@ QUnit.test('should hide element if isVisible is false before element is created'

ok(view.$().is(':hidden'), 'should be hidden');

run(() => view.remove());
run(() => view.destroyElement());

run(() => set(view, 'isVisible', true));

run(() => view.append());

ok(view.$().is(':visible'), 'view should be visible');

run(() => view.remove());
run(() => view.destroyElement());

deepEqual(warnings, [], 'no warnings were triggered');
});
Expand All @@ -84,7 +84,7 @@ QUnit.test('should hide views when isVisible is a CP returning false', function(
run(() => set(view, 'isVisible', true));

ok(view.$().is(':visible'), 'the view is visible');
run(() => view.remove());
run(() => view.destroyElement());

deepEqual(warnings, [], 'no warnings were triggered');
});
Expand Down
51 changes: 0 additions & 51 deletions packages/ember-views/tests/views/view/remove_test.js

This file was deleted.

0 comments on commit de3bc9a

Please sign in to comment.