-
Notifications
You must be signed in to change notification settings - Fork 975
Add new thumnbail pixels and unique variant for Duck Player pixels #5803
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
base: develop
Are you sure you want to change the base?
Add new thumnbail pixels and unique variant for Duck Player pixels #5803
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
duckplayer/duckplayer-impl/src/main/java/com/duckduckgo/duckplayer/impl/RealDuckPlayer.kt
Outdated
Show resolved
Hide resolved
pixel.fire(duckPlayerPixelName, parameters = pixelData) | ||
duckPlayerPixelNames?.forEach { (duckPlayerPixelName, type) -> | ||
val dataToSend = when { | ||
duckPlayerPixelName == DUCK_PLAYER_OVERLAY_YOUTUBE_CHOICE_UNIQUE -> mapOf("choice" to pixelName.split(".").last()) |
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.
We shouldn't do this, at this point, I'd rather either create a wrapper class with PixelName
, Type
, and params
, or just fire the necessary pixels from each when
branch
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 agree, this is a little confusing. Encapsulating each in their own class seems like a better approach.
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.
@mgurgel Logically this looks good, just the one change and I’ll approve: https://github.com/duckduckgo/Android/pull/5803/files#r2035411909
Task/Issue URL: https://app.asana.com/0/72649045549333/1209575117168996/f
Description
Steps to test this PR
Note
npm i github:duckduckgo/content-scope-scripts#pr-releases/pr-1603
and ensure a bottomSheet for Duck Player is shown on YouTube video pages instead of the previous overlayFeature 1
duckplayer_overlay_youtube_impressions
andduckplayer_overlay_youtube_impressions_unique
are firedduckplayer_overlay_youtube_impressions
is fired butduckplayer_overlay_youtube_impressions_unique
is notFeature 2
duckplayer_overlay_youtube_dismiss-play
is firedduckplayer_overlay_youtube_choice_unique
is fired with parameterchoice=dismiss
duckplayer_overlay_youtube_dismiss-play
is fired butduckplayer_overlay_youtube_choice_unique
is notYou will need to clear Duck Player user preferences at this point
Feature 3
duckplayer_overlay_youtube_watch_here
is firedduckplayer_overlay_youtube_choice_unique
is fired with parameterchoice=do_not_use
duckplayer_overlay_youtube_watch_here
is fired butduckplayer_overlay_youtube_choice_unique
is notYou will need to clear Duck Player user preferences at this point
Feature 4
duckplayer_view-from_youtube_main-overlay
is firedduckplayer_overlay_youtube_choice_unique
is fired with parameterchoice=use
duckplayer_view-from_youtube_main-overlay
is fired butduckplayer_overlay_youtube_choice_unique
is notUI changes
No UI changes