add component to specific index #2540

erikyuzwa · 2015-09-02T03:08:29Z

and tests

@heff my humble apologies for a PR this way. I did attempt to use contrib feature submit as per docs, but I was getting an ----->undefined as a response after answering all the prompts.

this corresponds with issue #2534

and tests

pam · 2015-09-02T03:12:52Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 80d8f26
Build details: https://travis-ci.org/pam/video.js/builds/78336230

(Please note that this is a fully automated comment.)

heff · 2015-09-02T20:10:36Z

A suggestion PR made here: https://github.com/erikyuzwa/video.js/pull/1

…th-index Minor changes to #2540

pam · 2015-09-02T20:37:03Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 19cad93
Build details: https://travis-ci.org/pam/video.js/builds/78467615

(Please note that this is a fully automated comment.)

pam · 2015-09-02T20:52:25Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 0527665
Build details: https://travis-ci.org/pam/video.js/builds/78469829

(Please note that this is a fully automated comment.)

heff · 2015-09-03T03:39:19Z

We can keep this open until we can fix the last broken test. Not sure if you meant to close it.

erikyuzwa · 2015-09-03T03:50:02Z

sounds good -- wasn't sure if you were getting annoying travis alerts or something because of the failed tests. ;)

erikyuzwa · 2015-09-03T05:23:49Z

after going through the sandbox dev server, I was able to make the failing test pass with:

ok(player.el().children[player.el().children.length-1].className.indexOf('vjs-tech') !== -1, 'media controller loaded');

the vjs-tech element was pushed to the tail of the Array rather than the head.

but is it 'right'?

pam · 2015-09-03T05:29:50Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: d763d0e
Build details: https://travis-ci.org/pam/video.js/builds/78520405

(Please note that this is a fully automated comment.)

heff · 2015-09-03T18:07:59Z

Yeah, the problem is we need the tech to be first, always, so that other components in the player overlap the video.

The problem is we're adding the tech directly to the player element instead of using the addChild function of the player. This means it doesn't get added to the children array or any of the child indexes. So now when we try to insert other elements into the right place using the index of the children array, it doesn't match up with the indexes of where the child elements actually are in the player element... I think the solution is to make loadTech use the addChild function, but it's a little more complicated than just switching it.

erikyuzwa · 2015-09-03T20:09:05Z

leave it to me to find the trickiest thing to tweak. I'm hoping the end result is an approach that really benefits users. I'll take a look at the player then and use your advice as a roadmap, many thanks.

heff · 2015-09-03T20:24:16Z

Thanks for being patient. :) We'll get this figured out.

erikyuzwa · 2015-09-04T02:13:48Z

aye

using the updated addChild to add our vjs-tech into our DOM children array

erikyuzwa · 2015-09-04T03:11:49Z

made use of the addChild method as per your recommendation. The vjs-tech element is now appearing first in the children array. (was that empty div always there?)

pam · 2015-09-04T03:14:36Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 0f02202
Build details: https://travis-ci.org/pam/video.js/builds/78692641

(Please note that this is a fully automated comment.)

erikyuzwa · 2015-09-18T14:15:07Z

hey @heff, I know it's a crazy month - just wondering if you've had a chance to look at my latest commit to see if this is in the right direction.

nickygerritsen · 2015-09-29T05:54:20Z

This PR makes me happy 😃.

Anyway, aren't the tests missing stuff that tests the el() is in the correct position?

erikyuzwa · 2015-09-29T13:58:43Z

thanks @nickygerritsen 👍 - yes great point, I shall work at that to try and see this PR through

nickygerritsen · 2015-10-13T06:59:48Z

@erikyuzwa any update on this :)?

nickygerritsen · 2015-10-18T18:51:54Z

Btw, would you also consider adding addChildBefore and addChildAfter? I think this makes sense with this index-based child adding and shouldn't be too hard?

Something like:

addChildBefore(child, before, options={}) {
  this.addChild(child, options, getChildIndex(before));
}

addChildAfter(child, after, options={}) {
  this.addChild(child, options, getChildIndex(after) + 1);
}

getChildIndex(child) {
  let component;
  if (typeof child === 'string') {
    component = this.getChild(child);
  } else {
    component = child;
  }

  let index = this.children_.indexOf(component);

  if (index === -1) { /* Throw some error? */ }

  return index;
}

Or similar.

I think it would make sense for something like this? Normally you want to insert a child at a given index because you want it to appear before / after some other component (actually exactly as you suggested in #2534).

erikyuzwa · 2015-11-10T02:56:31Z

hey @nickygerritsen - hmm perhaps to try and keep this PR as "small" as possible (and in motion), I'd suggest creating a PR of your own once / if this one goes through. Ultimately it'd be up to @heff and the videojs team.

erikyuzwa · 2016-01-17T03:34:47Z

Thanks @gkatsev - much appreciated. I'll take a look for that module. Once I find it, I might submit a PR on the readme to update that submission section to help others out.

gkatsev · 2016-01-17T05:34:06Z

Yeah, the contributing.md is kind of outdated right now. It's on my TODO to update it.

erikyuzwa · 2016-01-22T00:10:45Z

I get it. You're swamped, no worries.
If this is the correct contrib - https://www.npmjs.com/package/contrib - then I can submit a document PR myself.

gkatsev · 2016-02-02T19:48:05Z

src/js/component.js

@@ -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;
+      var refNode = childNodes[index] || undefined;


~~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.

gkatsev · 2016-02-02T19:59:48Z

@erikyuzwa hey, finally got around to reviewing it. Made a few comments. Thanks.

erikyuzwa · 2016-02-02T20:16:14Z

oh awesome - my humble thanks @gkatsev - always looking to improving my output. I'll take a look at your comments.

This reverts commit 6efa25e.

gkatsev · 2016-02-04T16:42:40Z

Thanks. LGTM.

dmlap · 2016-02-04T18:40:05Z

LGTM

gkatsev · 2016-02-04T18:43:25Z

@erikyuzwa Thanks!

gkatsev · 2016-02-04T18:46:55Z

And it's out in 5.7.0! (Not yet on the CDN, but available in github releases and npm as next tag).

erikyuzwa · 2016-02-04T18:58:16Z

most excellent - thanks @gkatsev and @dmlap for all the help!

properly add overlay as a player component so that video.js will be aware of its existence when initializing additional components; not inserting those before the overlay unless specified otherwise. See videojs/video.js#2540 Fixes #11

add component to specific index

80d8f26

and tests

Minor changes to #2540

ba0065a

Merge pull request #1 from heff/review-erikyuzwa-feature/add-child-wi…

19cad93

…th-index Minor changes to #2540

fixed addChild tests

0527665

erikyuzwa closed this Sep 3, 2015

erikyuzwa reopened this Sep 3, 2015

updated failing test with proper index

d763d0e

but is it 'right'?

erikyuzwa added 2 commits September 3, 2015 21:09

making use of component.addChild

3b0ba5f

using the updated addChild to add our vjs-tech into our DOM children array

restored original media controller test

0f02202

nickygerritsen mentioned this pull request Oct 26, 2015

Why plugins are Placed after the full-screen button at the end? #2731

Closed

gkatsev added enhancement minor This PR can be added to a minor release. It should not be added to a patch release. needs: LGTM Needs one or more additional approvals labels Jan 16, 2016

gkatsev reviewed Feb 2, 2016
View reviewed changes

erikyuzwa added 4 commits February 3, 2016 00:39

modified vars to lets

04fef83

undo the DOM el function append

6efa25e

Merge branch 'master' into feature/add-child-with-index

ccac3c5

Revert "undo the DOM el function append"

266097a

This reverts commit 6efa25e.

gkatsev added in progress and removed needs: LGTM Needs one or more additional approvals labels Feb 3, 2016

erikyuzwa added 2 commits February 3, 2016 13:43

updated comments to help clarify dom insertion

cb49200

reworked dom insertion to allow proper children

7b7ca3d

gkatsev added the needs: LGTM Needs one or more additional approvals label Feb 4, 2016

gkatsev removed the needs: LGTM Needs one or more additional approvals label Feb 4, 2016

gkatsev closed this in fc7a166 Feb 4, 2016

gkatsev mentioned this pull request Mar 28, 2016

Update vjs.Component.addChild to allow an index #2534

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add component to specific index #2540

add component to specific index #2540

erikyuzwa commented Sep 2, 2015

pam commented Sep 2, 2015

heff commented Sep 2, 2015

pam commented Sep 2, 2015

pam commented Sep 2, 2015

heff commented Sep 3, 2015

erikyuzwa commented Sep 3, 2015

erikyuzwa commented Sep 3, 2015

pam commented Sep 3, 2015

heff commented Sep 3, 2015

erikyuzwa commented Sep 3, 2015

heff commented Sep 3, 2015 via email

erikyuzwa commented Sep 4, 2015

erikyuzwa commented Sep 4, 2015

pam commented Sep 4, 2015

erikyuzwa commented Sep 18, 2015

nickygerritsen commented Sep 29, 2015

erikyuzwa commented Sep 29, 2015

nickygerritsen commented Oct 13, 2015

nickygerritsen commented Oct 18, 2015

erikyuzwa commented Nov 10, 2015

erikyuzwa commented Jan 17, 2016

gkatsev commented Jan 17, 2016

erikyuzwa commented Jan 22, 2016

gkatsev Feb 2, 2016

gkatsev commented Feb 2, 2016

erikyuzwa commented Feb 2, 2016

gkatsev commented Feb 4, 2016

dmlap commented Feb 4, 2016

gkatsev commented Feb 4, 2016

gkatsev commented Feb 4, 2016

erikyuzwa commented Feb 4, 2016

add component to specific index #2540

add component to specific index #2540

Conversation

erikyuzwa commented Sep 2, 2015

pam commented Sep 2, 2015

heff commented Sep 2, 2015

pam commented Sep 2, 2015

pam commented Sep 2, 2015

heff commented Sep 3, 2015

erikyuzwa commented Sep 3, 2015

erikyuzwa commented Sep 3, 2015

pam commented Sep 3, 2015

heff commented Sep 3, 2015

erikyuzwa commented Sep 3, 2015

heff commented Sep 3, 2015 via email

erikyuzwa commented Sep 4, 2015

erikyuzwa commented Sep 4, 2015

pam commented Sep 4, 2015

erikyuzwa commented Sep 18, 2015

nickygerritsen commented Sep 29, 2015

erikyuzwa commented Sep 29, 2015

nickygerritsen commented Oct 13, 2015

nickygerritsen commented Oct 18, 2015

erikyuzwa commented Nov 10, 2015

erikyuzwa commented Jan 17, 2016

gkatsev commented Jan 17, 2016

erikyuzwa commented Jan 22, 2016

gkatsev Feb 2, 2016

Choose a reason for hiding this comment

gkatsev commented Feb 2, 2016

erikyuzwa commented Feb 2, 2016

gkatsev commented Feb 4, 2016

dmlap commented Feb 4, 2016

gkatsev commented Feb 4, 2016

gkatsev commented Feb 4, 2016

erikyuzwa commented Feb 4, 2016