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(Ads): Don't show duplicate SKIP UI in IMA CS #7084

Merged
merged 2 commits into from
Jul 23, 2024

Conversation

avelad
Copy link
Member

@avelad avelad commented Jul 22, 2024

@avelad avelad requested a review from theodab July 22, 2024 10:29
@avelad avelad added type: bug Something isn't working correctly component: UI The issue involves the Shaka Player UI component: ads The issue involves the Shaka Player ads API or the use of other ad SDKs priority: P1 Big impact or workaround impractical; resolve before feature release labels Jul 22, 2024
@avelad avelad added this to the v4.11 milestone Jul 22, 2024
@shaka-bot
Copy link
Collaborator

Incremental code coverage: 81.82%

Copy link
Contributor

@theodab theodab left a comment

Choose a reason for hiding this comment

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

I don't think I like this as a solution. It puts the burden of knowing the implementation details of different ad types into the UI library, instead of the ads code where it belongs. It also means that if we add another type of ads that have a built-in skip UI, that if condition will become longer and more complicated.

I'd prefer if, instead, you added a method with a name like needsSkipUI that returns false for client side ads and true for everything else, and then the UI can just query that.

@avelad avelad requested a review from theodab July 22, 2024 12:37
@avelad avelad merged commit 9337143 into shaka-project:main Jul 23, 2024
13 of 16 checks passed
@avelad avelad deleted the ui-skip branch July 23, 2024 06:24
@shaka-bot shaka-bot added the status: archived Archived and locked; will not be updated label Sep 21, 2024
@shaka-project shaka-project locked as resolved and limited conversation to collaborators Sep 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: ads The issue involves the Shaka Player ads API or the use of other ad SDKs component: UI The issue involves the Shaka Player UI priority: P1 Big impact or workaround impractical; resolve before feature release status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants