duckduckgo / Android

DuckDuckGo Android App
https://play.google.com/store/apps/details?id=com.duckduckgo.mobile.android
Apache License 2.0
3.63k stars 871 forks source link

Move Home Panel CTA to Bottom Sheet #4646

Closed malmstein closed 1 week ago

malmstein commented 2 weeks ago

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

Widget CTA Logic

Survey CTA Removal

malmstein commented 2 weeks ago

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

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

nalcalag commented 2 weeks ago

@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 commented 2 weeks ago

@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 commented 1 week ago

Thanks @nalcalag !