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.

Sign up for GitHub

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

Jump to bottom

add component to specific index #2540

Closed
wants to merge 18 commits into from
Closed

add component to specific index #2540

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
80d8f26
add component to specific index
erikyuzwa Sep 1, 2015
ba0065a
Minor changes to #2540
heff Sep 2, 2015
19cad93
Merge pull request #1 from heff/review-erikyuzwa-feature/add-child-wi…
erikyuzwa Sep 2, 2015
0527665
fixed addChild tests
erikyuzwa Sep 2, 2015
d763d0e
updated failing test with proper index
erikyuzwa Sep 3, 2015
3b0ba5f
making use of component.addChild
erikyuzwa Sep 4, 2015
0f02202
restored original media controller test
erikyuzwa Sep 4, 2015
5f02d1f
rewrote child node insertion into dom
erikyuzwa Nov 10, 2015
4e91e67
Merge branch 'master' into feature/add-child-with-index
erikyuzwa Jan 12, 2016
c5442c8
updated for webstorm ide
erikyuzwa Jan 12, 2016
d25a874
fixed commented test
erikyuzwa Jan 16, 2016
579215c
Merge branch 'master' into feature/add-child-with-index
erikyuzwa Jan 16, 2016
04fef83
modified vars to lets
erikyuzwa Feb 3, 2016
6efa25e
undo the DOM el function append
erikyuzwa Feb 3, 2016
ccac3c5
Merge branch 'master' into feature/add-child-with-index
erikyuzwa Feb 3, 2016
266097a
Revert "undo the DOM el function append"
erikyuzwa Feb 3, 2016
cb49200
updated comments to help clarify dom insertion
erikyuzwa Feb 3, 2016
7b7ca3d
reworked dom insertion to allow proper children
erikyuzwa Feb 3, 2016
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
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,5 @@ test/coverage/*
.sass-cache

dist/*

.idea/
9 changes: 6 additions & 3 deletions src/js/component.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,10 +336,11 @@ class Component {
*
* @param {String|Component} child The class name or instance of a child to add
* @param {Object=} options Options, including options to be passed to children of the child.
* @param {Number} index into our children array to attempt to add the child
* @return {Component} The child component (created by this process if a string was used)
* @method addChild
*/
addChild(child, options={}) {
addChild(child, options={}, index=this.children_.length) {
let component;
let componentName;

Expand Down Expand Up @@ -388,7 +389,7 @@ class Component {
component = child;
}

this.children_.push(component);
this.children_.splice(index, 0, component);

if (typeof component.id === 'function') {
this.childIndex_[component.id()] = component;
Expand All @@ -405,7 +406,9 @@ class Component {
// Add the UI object's element to the container div (box)
// Having an element is not required
if (typeof component.el === 'function' && component.el()) {
this.contentEl().appendChild(component.el());
var childNodes = this.contentEl().children;
Copy link
Member

Choose a reason for hiding this comment

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

please change these vars to lets.

var refNode = childNodes[index] || undefined;
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't refNode end up pointing at the current element? oh, I see now that it would be pointing at the current position in the elements list so it would be exactly the item we want to use here.

Also, it would automatically get the value undefined if childNodes[index] doesn't exist. It would be better to use || null.

this.contentEl().insertBefore(component.el(), refNode);
}

// Return so it can stored on parent object if desired.
Expand Down
12 changes: 11 additions & 1 deletion src/js/player.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,17 @@ class Player extends Component {
if (tag.parentNode) {
tag.parentNode.insertBefore(el, tag);
}
Dom.insertElFirst(tag, el); // Breaks iPhone, fixed in HTML5 setup.

//Dom.insertElFirst(tag, el); // Breaks iPhone, fixed in HTML5 setup.

// append the DOM el function to make use of our component.addChild method
// to properly be injected into the children Array
tag.el = function () {
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer not to modify the DOM element as much as we can. I would prefer this wasn't changed quite yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

understood @gkatsev - just not sure where this came from. I have a feeling it was @heff during his original input into this branch. Considering the player.test.js was also updated to support this. If I go with your suggestion and undo this, it breaks the unit test.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, interesting. I'll take a look later today.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like the issue is that for some reason if we're using Dom.insertElFirst here, the video element ends up becoming the last child of the player div. If we're doing this tag.el() thing, it ends up working correctly.
Investigating.

Copy link
Member

Choose a reason for hiding this comment

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

It's because addChild defaults the insertion index to the length of the children array but if we insert the element manually, it won't end up in the children array so items will end up getting inserted before it into the DOM.
The workaround would be to insert it manually into the dom and then insert it manually into the children's array:

Dom.insertElFirst(tag, el); // Breaks iPhone, fixed in HTML5 setup.
this.children_.unshift(tag);

Copy link
Member

Choose a reason for hiding this comment

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

If you can make that last change, I think we may be able to pull it in this week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good. updated the comments there to try and capture the learnings

Copy link
Member

Choose a reason for hiding this comment

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

I think we should update it to be something like:

// insert the tag as the first child of the player element
// then manually add it to the children array so that this.addChild 
// will work properly for other components
Dom.insertElFirst(tag, el); // Breaks iPhone, fixed in HTML5 setup.
this.children_.unshift(tag);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh! my bad, I misunderstood. I'll update that straight away.

Copy link
Member

Choose a reason for hiding this comment

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

Heh, no worries. I realized I wasn't perfectly clear. I'll try and work on that for the future and would love to see more contributions from you as well!

return this;
};

// add our tag as the first child
this.addChild(tag, {}, 0);

this.el_ = el;

Expand Down
31 changes: 30 additions & 1 deletion test/unit/component.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,43 @@ test('should add a child component', function(){
ok(comp.getChildById(child.id()) === child);
});

test('should add a child component to an index', function(){
var comp = new Component(getFakePlayer());

var child = comp.addChild('component');

ok(comp.children().length === 1);
ok(comp.children()[0] === child);

var child0 = comp.addChild('component', {}, 0);
ok(comp.children().length === 2);
ok(comp.children()[0] === child0);
ok(comp.children()[1] === child);

var child1 = comp.addChild('component', {}, '2');
ok(comp.children().length === 3);
ok(comp.children()[2] === child1);

var child2 = comp.addChild('component', {}, undefined);
ok(comp.children().length === 4);
ok(comp.children()[3] === child2);

var child3 = comp.addChild('component', {}, -1);
ok(comp.children().length === 5);
ok(comp.children()[3] === child3);
ok(comp.children()[4] === child2);
});

test('addChild should throw if the child does not exist', function() {
var comp = new Component(getFakePlayer());

throws(function() {
comp.addChild('non-existent-child');
comp.addChild('non-existent-child');
}, new Error('Component Non-existent-child does not exist'), 'addChild threw');

});


test('should init child components from options', function(){
var comp = new Component(getFakePlayer(), {
children: {
Expand Down