Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants