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

Move Home Panel CTA to Bottom Sheet #4646

Merged

Conversation

malmstein
Copy link
Contributor

@malmstein malmstein commented Jun 13, 2024

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

Description

Migrates HomeCTA to use PromoBottomSheet
We are also removing all the code to display Surveys in that CTA

Steps to test this PR

Widget CTA Orientation

  • Fresh install, finish onboarding and create a New Tab
  • Verify that Home Widget Bottom Sheet is visible
  • Rotate the device and verify that Bottom Sheet is still visible

Widget CTA Logic

  • Fresh install, finish onboarding and create a New Tab
  • Verify that Home Widget Bottom Sheet is visible
  • Dismiss it by tapping anywhere in the screen
  • Close the app and open it again
  • Verify that Bottom Sheet doesn’t appear again

Survey CTA Removal

  • Create sandbox survey
  • Change endpoint to trigger surveys from sandbox
  • Fresh install
  • Verify that Survey CTA is not visible
  • Verify app works as expected

@malmstein
Copy link
Contributor Author

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

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

@malmstein malmstein marked this pull request as ready for review June 13, 2024 20:37
@malmstein malmstein requested a review from nalcalag as a code owner June 13, 2024 20:37
@malmstein malmstein mentioned this pull request Jun 13, 2024
2 tasks
@malmstein malmstein force-pushed the feature/david/06-13-move_home_panel_cta_to_bottom_sheet branch 2 times, most recently from 27bd6fd to 39a2f97 Compare June 14, 2024 09:44
@nalcalag nalcalag self-assigned this Jun 14, 2024
@nalcalag
Copy link
Contributor

@malmstein if I rotate the screen following the steps to test, it crashes.
When fun renderHomeCta() is called after onConfigurationChanged(), we get the exception lateinit property ctaBottomSheet has not been initialised.
Ping me if you need anything.

@nalcalag
Copy link
Contributor

@malmstein I tested this and works as expected 👍 However, I have a couple of concerns with the change:

  1. When the BottomSheet is displayed, the rest of the browser is greyed out, and users can't interact with the browser until they dismiss the CTA. Therefore, I think we should change it as it was before in terms of user experience.
  2. The CTA display should be handled from the Cta.kt class instead of directly from the BrowserTabFragment for consistency and to extract code from the Fragment. We can create a new interface BottomSheetCta which would have buildCta() and return the instance of the PromoBottomSheetDialog. Then make HomePanelCta to implement this new interface instead ViewCta. (Just an idea)

I'll include this comment in Asana too so we can discuss it there.

@malmstein malmstein force-pushed the feature/david/06-13-move_home_panel_cta_to_bottom_sheet branch from 41141ae to d347ee4 Compare June 18, 2024 09:55
@malmstein malmstein force-pushed the feature/david/06-13-move_home_panel_cta_to_bottom_sheet branch from 47a827f to 6fe8009 Compare June 19, 2024 10:49
Copy link
Contributor

@nalcalag nalcalag left a comment

Choose a reason for hiding this comment

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

Tested and works as expected ✅

There were a couple of comments from the previous review:

  1. (NIT) add description to the new method in the PromoBottomSheetDialog API
  2. (not a blocker) Move logic to display the PromoBottomSheet to the Cta.kt instead the BrowserTabFragment

@malmstein
Copy link
Contributor Author

Thanks @nalcalag !

@malmstein malmstein merged commit 42ac728 into develop Jun 19, 2024
6 checks passed
@malmstein malmstein deleted the feature/david/06-13-move_home_panel_cta_to_bottom_sheet branch June 19, 2024 13:53
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.

None yet

2 participants