-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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(Ads): Allow play interstitials on iOS fullscreen #7538
Conversation
lib/ads/interstitial_ad_manager.js
Outdated
/** | ||
* @private | ||
*/ | ||
checkSupportForMultipleMediaElements_() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how happy I am that this method is called checkSupportForMultipleMediaElements_
, but the variable that it mutates is called usingBaseVideo_
. It seems non-obvious to me.
I can see three possible changes:
- Rename the variable.
usingMultipleMediaElements_
? - Rename the method.
determineIfUsingBaseVideo_
? - Add a JSDoc description mentioning that this method sets
usingBaseVideo_
.
Of those options I feel like 2 is the best, but I could see arguments for 1 or 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done! (2nd option)
No description provided.