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

Update initial setup message for Duck Player page #4752

Conversation

CrisBarreiro
Copy link
Contributor

@CrisBarreiro CrisBarreiro commented Jul 11, 2024

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

Description

Steps to test this PR

https://app.asana.com/0/0/1207704461779423/f

Feature 1

  • Smoke test duck player

Open settings

  • Open a video in Duck Player
  • Press the settings button in the Duck Player page
  • Check you're redirected to settings

@CrisBarreiro CrisBarreiro mentioned this pull request Jul 11, 2024
18 tasks
Copy link
Contributor Author

CrisBarreiro commented Jul 11, 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

Copy link
Collaborator

@shakyShane shakyShane left a comment

Choose a reason for hiding this comment

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

Thanks :)

@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duckplayer/native-settings branch from 40ab3e7 to 0a92af2 Compare July 11, 2024 15:43
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/update-initial-setup-duck-player-page branch from e7ec676 to 741042d Compare July 11, 2024 15:43
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duckplayer/native-settings branch from 0a92af2 to 1e20bc1 Compare July 11, 2024 15:48
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/update-initial-setup-duck-player-page branch from 741042d to 77a1bdb Compare July 11, 2024 15:48
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duckplayer/native-settings branch from 1e20bc1 to d3483e0 Compare July 11, 2024 15:51
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/update-initial-setup-duck-player-page branch 2 times, most recently from c9639aa to 1be170e Compare July 11, 2024 16:05
@@ -69,6 +70,7 @@ class ContentScopeScriptsJsMessaging @Inject constructor(
webView.url?.toUri()?.host
}
jsMessage?.let {
Timber.d("Cris Processing message: $it")
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: Just in case you forget to remove this log.

@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duckplayer/native-settings branch from d3483e0 to 4a6069c Compare July 19, 2024 11:36
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/update-initial-setup-duck-player-page branch from f1130da to 2057954 Compare July 19, 2024 11:36
@CrisBarreiro CrisBarreiro mentioned this pull request Jul 31, 2024
3 tasks
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duckplayer/native-settings branch from 4a6069c to 1c70aab Compare August 7, 2024 09:56
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/update-initial-setup-duck-player-page branch from 40fc837 to d9fa7be Compare August 7, 2024 09:56
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duckplayer/native-settings branch from 1c70aab to f27c311 Compare August 7, 2024 10:28
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/update-initial-setup-duck-player-page branch 2 times, most recently from 8242a9f to fab0899 Compare August 7, 2024 13:16
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/update-initial-setup-duck-player-page branch from fab0899 to 8d68e61 Compare August 8, 2024 11:44
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/duckplayer/native-settings branch from ef92132 to 4330616 Compare August 8, 2024 11:50
@CrisBarreiro CrisBarreiro force-pushed the feature/cris/update-initial-setup-duck-player-page branch from 8d68e61 to 1f34224 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.

Overall I'd like to see more tests before merging. We are introducing changes to the view model, creating a helper and adding messages that are not tested.

@CrisBarreiro CrisBarreiro force-pushed the feature/cris/update-initial-setup-duck-player-page branch from 1f34224 to 878ad02 Compare August 20, 2024 15:28
@CrisBarreiro CrisBarreiro changed the base branch from feature/cris/duckplayer/native-settings to feature/cris/duckplayer/create-module August 20, 2024 15:28
@CrisBarreiro CrisBarreiro merged commit 878ad02 into feature/cris/duckplayer/create-module Aug 20, 2024
5 of 6 checks passed
@CrisBarreiro CrisBarreiro deleted the feature/cris/update-initial-setup-duck-player-page branch August 20, 2024 15:37
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.

4 participants