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 Duck Player prime modal #4824

Conversation

CrisBarreiro
Copy link
Contributor

@CrisBarreiro CrisBarreiro commented Jul 31, 2024

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

Description

Steps to test this PR

Feature 1

  • Open Duck Player
  • Click Info button
  • Check prime modal is correctly shown in both landscape and portrait

UI changes

Screenshot_20240802_180603

Screenshot_20240802_180616

Copy link
Contributor Author

CrisBarreiro commented Jul 31, 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-bottom-sheet branch 2 times, most recently from 92086a5 to 9c50362 Compare August 2, 2024 10:46
@CrisBarreiro CrisBarreiro marked this pull request as ready for review August 2, 2024 16:07
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player-bottom-sheet branch from b8170e3 to 3d1bff8 Compare August 2, 2024 16:09
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player-bottom-sheet branch 2 times, most recently from edd7f0e to 9fd2a15 Compare August 2, 2024 16:44
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/navigate-back-from-duck-player branch from 30fced9 to 5172524 Compare August 5, 2024 08:55
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player-bottom-sheet branch from 9fd2a15 to 23fc362 Compare August 5, 2024 08:55
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/navigate-back-from-duck-player branch from 5172524 to 8789ab0 Compare August 5, 2024 09:42
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player-bottom-sheet branch from 23fc362 to 0b7c89c Compare August 5, 2024 09:42
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/navigate-back-from-duck-player branch from 8789ab0 to 8c40743 Compare August 7, 2024 09:56
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player-bottom-sheet branch from 0b7c89c to 8b9dafd Compare August 7, 2024 09:57
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/navigate-back-from-duck-player branch from 8c40743 to 1865035 Compare August 7, 2024 10:28
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player-bottom-sheet branch from 8b9dafd to e136a14 Compare August 7, 2024 10:28
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/navigate-back-from-duck-player branch from 1865035 to 742ea02 Compare August 7, 2024 13:17
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player-bottom-sheet branch from e136a14 to e815f2e Compare August 7, 2024 13:17
@marcosholgado
Copy link
Contributor

One problem that I'm seeing on my S10 Android 12. When the screen is in landscape the X to close the popup is over/below the bottom bar and the X is unclickable.

Screenshot 2024-08-07 at 16 24 23

@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player-bottom-sheet branch from e815f2e to 7f8dc5a Compare August 7, 2024 16:09
@CrisBarreiro
Copy link
Contributor Author

One problem that I'm seeing on my S10 Android 12. When the screen is in landscape the X to close the popup is over/below the bottom bar and the X is unclickable.

Screenshot 2024-08-07 at 16 24 23

@marcosholgado can you try again? I've made some changes but don't have any Samsung devices to test on

@CrisBarreiro CrisBarreiro force-pushed the feature/cris/navigate-back-from-duck-player branch from 742ea02 to ae1e3be Compare August 8, 2024 11:44
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player-bottom-sheet branch from 7f8dc5a to f6be548 Compare August 8, 2024 11:44
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/navigate-back-from-duck-player branch from ae1e3be to 54cc443 Compare August 8, 2024 11:50
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player-bottom-sheet branch from f6be548 to 4f59bc3 Compare August 8, 2024 11:50
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, left just two nits.

@@ -168,6 +169,9 @@ class DuckPlayerJSHelper @Inject constructor(
"openSettings" -> {
return OpenDuckPlayerSettings
}
"openInfo" -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any tests for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested in next PR, #4872

@@ -133,6 +133,7 @@ class ContentScopeScriptsJsMessaging @Inject constructor(
"sendDuckPlayerPixel",
"setUserValues",
"openDuckPlayer",
"openInfo",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we test this type of message too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested in next PR, #4872

@marcosholgado marcosholgado self-assigned this Aug 14, 2024
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/navigate-back-from-duck-player branch from 54cc443 to 0a98f10 Compare August 20, 2024 11:09
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player-bottom-sheet branch from 4f59bc3 to 9797a52 Compare August 20, 2024 11:09
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/navigate-back-from-duck-player branch from 0a98f10 to 2d3b27f Compare August 20, 2024 11:14
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player-bottom-sheet branch from 9797a52 to 864511c Compare August 20, 2024 11:14
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/navigate-back-from-duck-player branch from 2d3b27f to 9539c9c Compare August 20, 2024 15:28
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duck-player-bottom-sheet branch from 864511c to 4930428 Compare August 20, 2024 15:28
Base automatically changed from feature/cris/navigate-back-from-duck-player to feature/cris/duckplayer/create-module August 20, 2024 15:40
@CrisBarreiro CrisBarreiro merged commit 4930428 into feature/cris/duckplayer/create-module Aug 20, 2024
4 of 5 checks passed
@CrisBarreiro CrisBarreiro deleted the feature/cris/duck-player-bottom-sheet branch August 20, 2024 15:41
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