From ccd145210d007fc3a065ace5a12d0eb86f86ca0c Mon Sep 17 00:00:00 2001 From: Kris Selden Date: Wed, 29 Jun 2016 12:11:56 -0700 Subject: [PATCH] [CLEANUP] reducing private API surface of views. * 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. --- packages/ember-htmlbars/lib/renderer.js | 5 +- .../lib/mixins/child_views_support.js | 1 - .../ember-views/lib/mixins/view_support.js | 63 ++----------------- .../lib/mixins/visibility_support.js | 22 +++---- .../ember-views/tests/views/checkbox_test.js | 4 +- .../tests/views/view/append_to_test.js | 2 +- .../views/view/class_name_bindings_test.js | 2 +- .../tests/views/view/is_visible_test.js | 8 +-- .../tests/views/view/remove_test.js | 51 --------------- 9 files changed, 25 insertions(+), 133 deletions(-) delete mode 100644 packages/ember-views/tests/views/view/remove_test.js diff --git a/packages/ember-htmlbars/lib/renderer.js b/packages/ember-htmlbars/lib/renderer.js index 081e505f93e..98647acba06 100755 --- a/packages/ember-htmlbars/lib/renderer.js +++ b/packages/ember-htmlbars/lib/renderer.js @@ -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'); @@ -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 }) { diff --git a/packages/ember-views/lib/mixins/child_views_support.js b/packages/ember-views/lib/mixins/child_views_support.js index d87e8de09b3..48370b982eb 100644 --- a/packages/ember-views/lib/mixins/child_views_support.js +++ b/packages/ember-views/lib/mixins/child_views_support.js @@ -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 diff --git a/packages/ember-views/lib/mixins/view_support.js b/packages/ember-views/lib/mixins/view_support.js index 2151a36dd89..75e55488742 100644 --- a/packages/ember-views/lib/mixins/view_support.js +++ b/packages/ember-views/lib/mixins/view_support.js @@ -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'; @@ -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; } }, @@ -47,14 +47,15 @@ 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; } }, @@ -110,21 +111,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. @@ -264,25 +250,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): @@ -557,24 +524,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 diff --git a/packages/ember-views/lib/mixins/visibility_support.js b/packages/ember-views/lib/mixins/visibility_support.js index f5c180288b2..b771807e5dc 100644 --- a/packages/ember-views/lib/mixins/visibility_support.js +++ b/packages/ember-views/lib/mixins/visibility_support.js @@ -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; } }); diff --git a/packages/ember-views/tests/views/checkbox_test.js b/packages/ember-views/tests/views/checkbox_test.js index 99c16b974e8..b8696c31e03 100644 --- a/packages/ember-views/tests/views/checkbox_test.js +++ b/packages/ember-views/tests/views/checkbox_test.js @@ -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()); diff --git a/packages/ember-views/tests/views/view/append_to_test.js b/packages/ember-views/tests/views/view/append_to_test.js index 831bb8d0289..2d9091ff26d 100644 --- a/packages/ember-views/tests/views/view/append_to_test.js +++ b/packages/ember-views/tests/views/view/append_to_test.js @@ -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'); diff --git a/packages/ember-views/tests/views/view/class_name_bindings_test.js b/packages/ember-views/tests/views/view/class_name_bindings_test.js index 058527bec66..3e805e40976 100644 --- a/packages/ember-views/tests/views/view/class_name_bindings_test.js +++ b/packages/ember-views/tests/views/view/class_name_bindings_test.js @@ -26,7 +26,7 @@ QUnit.skip('classNameBindings lifecycle test', function() { equal(isWatching(view, 'priority'), true); run(() => { - view.remove(); + view.destroyElement(); view.set('priority', 'low'); }); diff --git a/packages/ember-views/tests/views/view/is_visible_test.js b/packages/ember-views/tests/views/view/is_visible_test.js index a45330cc6e2..d8912c7f75a 100644 --- a/packages/ember-views/tests/views/view/is_visible_test.js +++ b/packages/ember-views/tests/views/view/is_visible_test.js @@ -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'); }); @@ -57,7 +57,7 @@ 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)); @@ -65,7 +65,7 @@ QUnit.test('should hide element if isVisible is false before element is created' ok(view.$().is(':visible'), 'view should be visible'); - run(() => view.remove()); + run(() => view.destroyElement()); deepEqual(warnings, [], 'no warnings were triggered'); }); @@ -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'); }); diff --git a/packages/ember-views/tests/views/view/remove_test.js b/packages/ember-views/tests/views/view/remove_test.js deleted file mode 100644 index 88fe70dad74..00000000000 --- a/packages/ember-views/tests/views/view/remove_test.js +++ /dev/null @@ -1,51 +0,0 @@ -import { get } from 'ember-metal/property_get'; -import run from 'ember-metal/run_loop'; -import jQuery from 'ember-views/system/jquery'; -import View from 'ember-views/views/view'; - -let parentView, child; - -let view; -// ....................................................... -// removeFromParent() -// -QUnit.module('View#removeFromParent', { - teardown() { - run(() => { - if (parentView) { parentView.destroy(); } - if (child) { child.destroy(); } - if (view) { view.destroy(); } - }); - } -}); - -QUnit.test('does nothing if not in parentView', function() { - child = View.create(); - - // monkey patch for testing... - ok(!get(child, 'parentView'), 'precond - has no parent'); - - child.removeFromParent(); - - run(() => child.destroy()); -}); - -QUnit.test('the DOM element is gone after doing append and remove in two separate runloops', function() { - view = View.create(); - run(() => view.append()); - run(() => view.remove()); - - let viewElem = jQuery('#' + get(view, 'elementId')); - ok(viewElem.length === 0, 'view\'s element doesn\'t exist in DOM'); -}); - -QUnit.test('the DOM element is gone after doing append and remove in a single runloop', function() { - view = View.create(); - run(() => { - view.append(); - view.remove(); - }); - - let viewElem = jQuery('#' + get(view, 'elementId')); - ok(viewElem.length === 0, 'view\'s element doesn\'t exist in DOM'); -});