Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Adding a removeChildren method #4

Merged
merged 2 commits into from Apr 2, 2013

Conversation

Projects
None yet
2 participants
Contributor

timmmichaud commented Mar 29, 2013

  • Remove all children, removing the name references and index
  • This method calls removeFromParent on the children so it also removes the child's DOM.
Adding a removeAllChildren method
- Remove all children, removing the name references and index
- This method calls removeFromParent on the children so it also removes the child's DOM.

@robrobbins robrobbins commented on an outdated diff Mar 31, 2013

container/container.js
@@ -86,6 +86,18 @@ sudo.Container.prototype.removeChild = function removeChild(arg) {
this._indexChildren_(i);
return this;
};
+
+// ###removeAllChildren
+// Remove all children, removing the name references and index
+// This method calls removeFromParent on the children so it also removes the child's DOM.
+// `returns` {Object} `this`
+sudo.Container.prototype.removeAllChildren = function removeAllChildren(arg) {
@robrobbins

robrobbins Mar 31, 2013

Contributor

let's call this 'removeChildren' instead. the 'all' is unnecessary.

@robrobbins robrobbins commented on an outdated diff Mar 31, 2013

container/container.js
@@ -86,6 +86,18 @@ sudo.Container.prototype.removeChild = function removeChild(arg) {
this._indexChildren_(i);
return this;
};
+
+// ###removeAllChildren
+// Remove all children, removing the name references and index
+// This method calls removeFromParent on the children so it also removes the child's DOM.
@robrobbins

robrobbins Mar 31, 2013

Contributor

Nope. Only the DataView class actually removes its DOM when removeFromParent is called (they override the base implementation provided by this object), classes inheriting from container (View, ViewController) do not (unless overridden).

@robrobbins robrobbins commented on an outdated diff Mar 31, 2013

container/container.js
@@ -86,6 +86,18 @@ sudo.Container.prototype.removeChild = function removeChild(arg) {
this._indexChildren_(i);
return this;
};
+
+// ###removeAllChildren
+// Remove all children, removing the name references and index
+// This method calls removeFromParent on the children so it also removes the child's DOM.
+// `returns` {Object} `this`
+sudo.Container.prototype.removeAllChildren = function removeAllChildren(arg) {
+ while(this.children.length > 0) {
@robrobbins

robrobbins Mar 31, 2013

Contributor

the GT operator isn't needed as we can rely on implicit type conversion here:

while(this.children.length) {

@robrobbins robrobbins commented on an outdated diff Mar 31, 2013

container/container.js
@@ -86,6 +86,18 @@ sudo.Container.prototype.removeChild = function removeChild(arg) {
this._indexChildren_(i);
return this;
};
+
+// ###removeAllChildren
+// Remove all children, removing the name references and index
+// This method calls removeFromParent on the children so it also removes the child's DOM.
+// `returns` {Object} `this`
+sudo.Container.prototype.removeAllChildren = function removeAllChildren(arg) {
+ while(this.children.length > 0) {
+ this.children[0].removeFromParent();
@robrobbins

robrobbins Mar 31, 2013

Contributor

as removeFromParent calls back to removeChild passing itself, and because we are in the parent 'scope' here - we can be more efficient by simply calling removeChild, passing the child:

this.removeChild(this.children.shift());

removeChild can take an actual child as an arg, and there is no need to provide the index as the parent will figure that out.

Using shift will create a FIFO - which seems correct, although we could provide an option to pass pop so a FILO kinda thing can be done - i don`t see that as necessary (and a dev would prob need to be overriding these then anyway in that scenario)

@robrobbins robrobbins commented on an outdated diff Mar 31, 2013

container/spec/container.spec.js
@@ -45,6 +45,13 @@ describe('Sudo Container Class', function() {
expect(c.length).toBe(0);
});
+ it('Removes all children', function() {
+ container.addChild(child1, 'Galahad').addChild(child2, 'Robin');
+ expect(container.children.length).toBe(2);
+ container.removeAllChildren();
@robrobbins

robrobbins Mar 31, 2013

Contributor

after the change this would read:

container.removeChildren();

Adding a removeChildren method
- Remove all children, removing the name references and index
- This method calls removeFromParent on the children so it leaves it up to the child to remove itself in whatever way it needs to. IE: DataView's also remove their DOM.

robrobbins added a commit that referenced this pull request Apr 2, 2013

@robrobbins robrobbins merged commit cf66641 into master Apr 2, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment