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

Add pixels #4902

Conversation

CrisBarreiro
Copy link
Contributor

@CrisBarreiro CrisBarreiro commented Aug 14, 2024

Copy link
Contributor Author

CrisBarreiro commented Aug 14, 2024

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

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

Join @CrisBarreiro and the rest of your teammates on Graphite Graphite

@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player/pixels branch 3 times, most recently from dd7ac2e to dc3c086 Compare August 19, 2024 12:53
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player/pixels branch 2 times, most recently from 9e9d625 to 44ee890 Compare August 19, 2024 18:18
@CrisBarreiro CrisBarreiro mentioned this pull request Aug 19, 2024
1 task
@CrisBarreiro CrisBarreiro marked this pull request as ready for review August 20, 2024 09:31
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player/allow-first-video branch from c612fce to a5175df Compare August 20, 2024 10:42
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player/pixels branch 2 times, most recently from d2f5b62 to 8ab4c9a Compare August 20, 2024 10:57
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player/allow-first-video branch from a5175df to cc594ee Compare August 20, 2024 11:10
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player/pixels branch from 8ab4c9a to 7103e11 Compare August 20, 2024 11:10
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player/allow-first-video branch from cc594ee to a49ff12 Compare August 20, 2024 11:14
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player/pixels branch from 7103e11 to a098918 Compare August 20, 2024 11:14
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player/allow-first-video branch from a49ff12 to f270933 Compare August 20, 2024 15:28
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player/pixels branch from a098918 to 716649f Compare August 20, 2024 15:28
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player/allow-first-video branch from f270933 to dd3df3a Compare August 21, 2024 09:01
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player/pixels branch from 716649f to 8e5af21 Compare August 21, 2024 09:01
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player/allow-first-video branch from dd3df3a to 3e646bc Compare August 21, 2024 10:45
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player/pixels branch from 8e5af21 to d65ba65 Compare August 21, 2024 10:45
Copy link
Contributor

@marcosholgado marcosholgado left a comment

Choose a reason for hiding this comment

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

LGTM. I'd like to find a way to move most of the logic from the DuckPlayerJsHelper into the duck player module but that's ok for now.

Also I'm not sure if this is the expected behaviour so just noting it here for you to check. The duck-player_view-from_other pixel is fired after clicking turn on duck player on the overlay, I don't think it should be fired though but not 100% sure.

@CrisBarreiro
Copy link
Contributor Author

LGTM. I'd like to find a way to move most of the logic from the DuckPlayerJsHelper into the duck player module but that's ok for now.

Also I'm not sure if this is the expected behaviour so just noting it here for you to check. The duck-player_view-from_other pixel is fired after clicking turn on duck player on the overlay, I don't think it should be fired though but not 100% sure.

@marcosholgado you're right. That pixel shouldn't be fired

on #1. There are a couple of reasons why I decided to keep it on the app module

  • I think the fact we're using JS messages is an implementation detail on the browser, not Duck Player itsels
  • We can't access BrowserTabViewModelcommands from Duck Player. However, we could introduce some "commands" in Duck Player and then map them to the actual ones in BrowserTabViewModel

No strong opinions though

@marcosholgado
Copy link
Contributor

LGTM. I'd like to find a way to move most of the logic from the DuckPlayerJsHelper into the duck player module but that's ok for now.
Also I'm not sure if this is the expected behaviour so just noting it here for you to check. The duck-player_view-from_other pixel is fired after clicking turn on duck player on the overlay, I don't think it should be fired though but not 100% sure.

@marcosholgado you're right. That pixel shouldn't be fired

on #1. There are a couple of reasons why I decided to keep it on the app module

  • I think the fact we're using JS messages is an implementation detail on the browser, not Duck Player itsels
  • We can't access BrowserTabViewModelcommands from Duck Player. However, we could introduce some "commands" in Duck Player and then map them to the actual ones in BrowserTabViewModel

No strong opinions though

Fair

@CrisBarreiro
Copy link
Contributor Author

@marcosholgado the pixel was fired due to a regression in CSS. There's a new commit addressing it on the CSS branch

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

Successfully merging this pull request may close these issues.

2 participants