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

chore: additional vjs ad errors #8623

Merged
merged 1 commit into from
Mar 4, 2024
Merged

Conversation

wseymour15
Copy link
Contributor

Description

Add additional errors to the video.js error interface regarding ads.

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)
  • Reviewed by Two Core Contributors

@wseymour15 wseymour15 self-assigned this Feb 28, 2024
Copy link

codecov bot commented Feb 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.71%. Comparing base (a57b07a) to head (012f79b).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8623   +/-   ##
=======================================
  Coverage   82.71%   82.71%           
=======================================
  Files         113      113           
  Lines        7636     7636           
  Branches     1835     1835           
=======================================
  Hits         6316     6316           
  Misses       1320     1320           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wseymour15 wseymour15 marked this pull request as ready for review March 1, 2024 18:11
@wseymour15 wseymour15 merged commit 7ed47de into main Mar 4, 2024
13 checks passed
@wseymour15 wseymour15 deleted the chore/additional-vjs-ad-errors branch March 4, 2024 19:23
AdsPrerollError: 'ads-preroll-error',
AdsMidrollError: 'ads-midroll-error',
AdsPostrollError: 'ads-postroll-error',
AdsMacroReplacementFailed: 'ads-marco-replacement-failed',
Copy link
Contributor

Choose a reason for hiding this comment

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

@wseymour15 looks like there is a little typo ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good eye, PR for that update: #8628

It is not an urgent fix, so no need to rush a release for this one.

@thijstriemstra
Copy link
Contributor

thijstriemstra commented Mar 28, 2024

Interesting to see this being added, since it's a plugin/thirdparty library afaik (and shouldn't be "bloating" a core video.js).

@misteroneill
Copy link
Member

misteroneill commented Mar 28, 2024

Interesting to see this being added, since it's a plugin/thirdparty library afaik (and shouldn't be "bloating" a core video.js).

Yeah, we agree that it's not the ideal place for these constants, but was added in the interest of the simplicity of having a single mapping of error codes in Video.js and it had a fairly minimal impact on bundle size.

I think we will plan to build in a more configurable mechanism in the future. Ultimately, these would belong in a library like videojs-contrib-ads.

@thijstriemstra
Copy link
Contributor

thijstriemstra commented Mar 29, 2024

Thanks for the feedback.

in the interest of the simplicity

is always a red flag isn't it (when coming from a marketing department i'd imagine).

I think we will plan to build in a more configurable mechanism in the future. Ultimately, these would belong in a library like videojs-contrib-ads.

Nice! It seems to be a re-occuring theme though: #8634

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.

5 participants