Skip to content

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

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

CrisBarreiro
Copy link
Contributor

@CrisBarreiro CrisBarreiro commented Mar 20, 2025

Task/Issue URL: https://app.asana.com/0/72649045549333/1209575117168996/f

Description

Steps to test this PR

Note

  • This PR requires a specific version of CSS. Run 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 overlay
  • Some of these pixels are unique and therefore only fired once, so, in case of doubt, run a fresh install

Feature 1

  • Open YouTube and search for a video
  • Check duckplayer_overlay_youtube_impressions and duckplayer_overlay_youtube_impressions_unique are fired
  • Repeat the previous steps
  • Check duckplayer_overlay_youtube_impressions is fired but duckplayer_overlay_youtube_impressions_unique is not

Feature 2

  • Open YouTube and search for a video
  • Tap on the video tumbnail
  • Check duckplayer_overlay_youtube_dismiss-play is fired
  • Check duckplayer_overlay_youtube_choice_unique is fired with parameter choice=dismiss
  • Repeat the previous steps
  • Check duckplayer_overlay_youtube_dismiss-play is fired but duckplayer_overlay_youtube_choice_unique is not

You will need to clear Duck Player user preferences at this point

Feature 3

  • Open YouTube and search for a video
  • Tap on the "No Thanks" button
  • Check duckplayer_overlay_youtube_watch_here is fired
  • Check duckplayer_overlay_youtube_choice_unique is fired with parameter choice=do_not_use
  • Repeat the previous steps
  • Check duckplayer_overlay_youtube_watch_here is fired but duckplayer_overlay_youtube_choice_unique is not

You will need to clear Duck Player user preferences at this point

Feature 4

  • Open YouTube and search for a video
  • Tap on the "Turn On Duck Player" button
  • Check duckplayer_view-from_youtube_main-overlay is fired
  • Check duckplayer_overlay_youtube_choice_unique is fired with parameter choice=use
  • Repeat the previous steps
  • Check duckplayer_view-from_youtube_main-overlay is fired but duckplayer_overlay_youtube_choice_unique is not

UI changes

No UI changes

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

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())
Copy link
Contributor Author

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

Copy link
Contributor

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.

@mgurgel mgurgel self-assigned this Apr 7, 2025
@mgurgel mgurgel marked this pull request as ready for review April 7, 2025 18:58
@mgurgel mgurgel requested a review from joshliebe April 7, 2025 18:58
Copy link
Contributor

@joshliebe joshliebe left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants