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(shadow-dom): prevent warning 'element supplied is not included' #8192

Merged
merged 8 commits into from
Jun 6, 2023
Merged

fix(shadow-dom): prevent warning 'element supplied is not included' #8192

merged 8 commits into from
Jun 6, 2023

Conversation

BrainCrumbz
Copy link
Contributor

Description

Addresses #8136 . It prevents a misleading warning when video element is created within a custom element.

Specific Changes proposed

Please list the specific changes involved in this pull request.

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 (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin).
      No codepen example created. Instead there's a dedicated sandbox example.
  • Reviewed by Two Core Contributors

@welcome
Copy link

welcome bot commented Mar 14, 2023

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@BrainCrumbz BrainCrumbz changed the title Shadow dom no warning Shadow DOM: prevent warning 'element supplied is not included' Mar 14, 2023
@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Merging #8192 (f54975b) into main (e42b859) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head f54975b differs from pull request most recent head 50e19d4. Consider uploading reports for the commit 50e19d4 to get more accurate results

@@           Coverage Diff           @@
##             main    #8192   +/-   ##
=======================================
  Coverage   82.23%   82.23%           
=======================================
  Files         112      112           
  Lines        7430     7432    +2     
  Branches     1791     1792    +1     
=======================================
+ Hits         6110     6112    +2     
  Misses       1320     1320           
Impacted Files Coverage Δ
src/js/video.js 94.16% <100.00%> (+0.09%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

Nice, code changes look good to me and huge thank you for providing an example in addition to a test! 💯

One minor nitpick, but otherwise this looks good to me.

package.json Show resolved Hide resolved
@misteroneill misteroneill added the needs: LGTM Needs one or more additional approvals label Apr 4, 2023
@misteroneill
Copy link
Member

It would be nice if the sample showed a playing video, but ultimately the change does seem to work. That error is not reported.

@BrainCrumbz
Copy link
Contributor Author

It would be nice if the sample showed a playing video, but ultimately the change does seem to work. That error is not reported.

@misteroneill yes, being lazy we set autoplay but that's not having effect. We're just now adding a click listener to play/pause.

@BrainCrumbz
Copy link
Contributor Author

@misteroneill just to mention that we've made it so that sandbox example can actually play

@mister-ben mister-ben changed the title Shadow DOM: prevent warning 'element supplied is not included' fix: Shadow DOM: prevent warning 'element supplied is not included' May 12, 2023
@misteroneill misteroneill removed the needs: LGTM Needs one or more additional approvals label May 12, 2023
@misteroneill misteroneill requested a review from gkatsev May 12, 2023 15:34
@BrainCrumbz
Copy link
Contributor Author

Hi all. Just wondering if there is any update on this, or something more is needed.
Thank you!

Copy link
Member

@misteroneill misteroneill left a comment

Choose a reason for hiding this comment

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

Thanks, sorry for the delay on this. I think it's ready to merge.

@misteroneill misteroneill changed the title fix: Shadow DOM: prevent warning 'element supplied is not included' fix(shadow-dom): prevent warning 'element supplied is not included' Jun 6, 2023
@misteroneill misteroneill merged commit dc1e2bb into videojs:main Jun 6, 2023
@welcome
Copy link

welcome bot commented Jun 6, 2023

Congrats on merging your first pull request! 🎉🎉🎉

@BrainCrumbz
Copy link
Contributor Author

Thanks, sorry for the delay on this. I think it's ready to merge.

Good news!
No worries, we were just afraid it could remain lost somehow.

Bye

@BrainCrumbz BrainCrumbz deleted the shadow-dom-no-warning branch June 7, 2023 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants