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.

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

fix: ensure components added with an index are added in the correct location #6297

Merged
merged 2 commits into from
Nov 14, 2019

Conversation

mister-ben
Copy link
Contributor

@mister-ben mister-ben commented Nov 3, 2019

Description

When an index is specified with Component.addChild() then the new component's element is added at the given index. This isn't necessarily correct, as not all of a component's children have DOM elements. Adding a component at the index of the control bar in the player's children will insert its el after the control bar el, because of the MediaLoader and LiveTracker.

Specific Changes proposed

When inserting the elelment, refer to the el_ of the sibling component this is to be inserted before.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

@gkatsev
Copy link
Member

gkatsev commented Nov 4, 2019

Took me a bit to figure out what was wrong here because the issue is so subtle. Is it possible that fixing this will cause issues from people inserting components at particular indexes?

@mister-ben
Copy link
Contributor Author

I only noticed this because the LiveTracker changed the position of an element added by a plugin. I was wondering if this could break anything, but I can't think of a reason to give an index at all other than to set the position in DOM.

src/js/component.js Outdated Show resolved Hide resolved
@misteroneill
Copy link
Member

Conceptually, I think this is a good change to make. There is some risk of causing problems for advanced use-cases that were relying on the broken behavior, but I think a strong argument can be made that, in this case, that wouldn't constitute a breaking change.

@gkatsev
Copy link
Member

gkatsev commented Nov 13, 2019

Yeah, makes sense. We can always revert it if we find this causes a major issue.

@gkatsev gkatsev merged commit f7b3772 into videojs:master Nov 14, 2019
@mister-ben mister-ben deleted the fix-component-index branch February 21, 2020 15:36
gkatsev pushed a commit that referenced this pull request May 26, 2020
…6644)

The fix in #6297 doesn't work where the child to insert before is an element rather than a component, e.g. the video element.
Check if the child to insert before is an element, as well as checking if it has an el_
DispatchCommit added a commit to bitwave-tv/video.js that referenced this pull request Jun 9, 2020
* fix: addChild with index should allow for children that are elements (videojs#6644)

The fix in videojs#6297 doesn't work where the child to insert before is an element rather than a component, e.g. the video element.
Check if the child to insert before is an element, as well as checking if it has an el_

* docs(README): Update CDN version urls (videojs#6658)

* fix(fs): don't set player element css props on native fullscreen (videojs#6673)

Fixes videojs#6640

* Update description of video.js in the package.json file, and add 'hls' keyword (videojs#6603)

* Update descritpion of video.js in the package.json file, and add 'hls' keyword

* Update package.json

Co-authored-by: Steve Heffernan <git@heff.me>

Co-authored-by: mister-ben <git@misterben.me>
Co-authored-by: Soroush Chehresa <s1996ch@gmail.com>
Co-authored-by: Gary Katsevman <git@gkatsev.com>
Co-authored-by: Owen Edwards <owen.r.edwards@gmail.com>
Co-authored-by: Steve Heffernan <git@heff.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants