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: addChild with index should allow for children that are elements #6644

Merged
merged 2 commits into from
May 26, 2020

Conversation

mister-ben
Copy link
Contributor

Description

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.

Specific Changes proposed

Check if the child to insert before is an element, as well as checking if it has an el_

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

@mister-ben mister-ben changed the title fix: addChild with index sohuld allow for children that are elements fix: addChild with index should allow for children that are elements May 12, 2020
@gkatsev
Copy link
Member

gkatsev commented May 12, 2020

Thanks @mister-ben. I think we'd probably want to release it as a patch against 7.7, 7.8 and master. Thoughts?

Copy link
Contributor

@evanfarina evanfarina left a comment

Choose a reason for hiding this comment

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

Thanks @mister-ben . I verified that this fixes a bug we've been having. For future reference, we were doing this from within a Component class:

player.addChild(this, {}, 0)

With the idea being we wanted to insert our component before the

@mister-ben
Copy link
Contributor Author

@gkatsev makes sense, i've branches off those tags ready once this is approved.

@evanfarina thanks for testing

@evanfarina
Copy link
Contributor

@mister-ben @gkatsev Any update as far as getting this in as a patch goes?

@gkatsev
Copy link
Member

gkatsev commented May 26, 2020

@evanfarina sorry, got distracted with other things. Merging and releasing now.

gkatsev added a commit that referenced this pull request May 26, 2020
…6671)

This is #6644 but against the 7.8.x release line.

Co-authored-by: mister-ben <[email protected]>
gkatsev added a commit that referenced this pull request May 26, 2020
…6672)

This is #6644 but against the 7.7.x release line.

Co-authored-by: mister-ben <[email protected]>
@gkatsev
Copy link
Member

gkatsev commented May 26, 2020

@evanfarina 7.7.7! is now out with this change. It's also in 7.8.2 if you felt like updating to that release line. It's also in master as of this change.

Thanks for the changes @mister-ben!

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 <[email protected]>

Co-authored-by: mister-ben <[email protected]>
Co-authored-by: Soroush Chehresa <[email protected]>
Co-authored-by: Gary Katsevman <[email protected]>
Co-authored-by: Owen Edwards <[email protected]>
Co-authored-by: Steve Heffernan <[email protected]>
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