-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Insert DefaultBrowser Cta and SearchWidget Cta as Dax dialog during Dax Journey #716
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
Insert DefaultBrowser Cta and SearchWidget Cta as Dax dialog during Dax Journey #716
Conversation
…Livedata. * This will remove a race condition bug when showing a new cta right after dismiss the previous one. Dismiss happens after a new cta has been emitted via livedata.
…alog class. Not all Dax dialogs use view highlighting.
This hierarchy will be changed.
…ialog Refacto dax dialog layout to use linearlayout instead of constraint layout
…n secondary button is clicked. Use same dialog when it has one or two buttons. Segregated logic only applies for dialogs highlighting a view.
Removing one layer of ConstraintLayout for one LinearLayout to simplify working with margins between buttons.
… from SecondaryButtonCta
* Introduce new logic to show/hide a secondaryButton on DaxDialogs. * Change DaxDialog creation logic inside Cta. * Adapt BrowserTabFragment listener logic to new chanes.
…ian/cta_concept_test_2 # Conflicts: # app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt # app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt # app/src/main/java/com/duckduckgo/app/cta/ui/Cta.kt # app/src/main/res/layout/content_dax_dialog.xml # app/src/main/res/values/attrs.xml # app/src/main/res/values/themes.xml
* show toast and background view to help user decide * if user clicks just once, we prompt the dialog again * navigation to settings extracted to a new class * Toast instructions extracted to a new class * Changed instructions card margins to mirror toolbar size * Show card only if ddg is not default browser
…r clicks on some Dax dialogs primary button
* Remove allocating users before referrer delay * Referrer variant is now based on DEFAULT_VARIANT * Adapt tests to new changes
This reverts commit 48ba2a7.
* Renamed some features making explicit to what part of the app they are related * Update test accordingly
…xp2' into feature/cristian/cta_concept_test_2
…feature/cristian/cta_concept_test_2 # Conflicts: # app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt # app/src/main/java/com/duckduckgo/app/browser/BrowserTabViewModel.kt
marcosholgado
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few things while testing this:
Dismissing the dialog sends dbo=dxdd but selecting another browser and coming back to DDG also sends dbo=dxdd. When analyzing this you won’t be able to know if the user selected a different browser or if they just dimissed the dialog. We used to send dbo=e for any external browser selected, you might want to consider this.
All the references in the description to m_db_s say the parameter sent should be dxdfs or dxdfd but we are sending dxs (no dx prefix), I guess is a typo in the description but better to double check.
This is my personal opinion so feel free to ignore this. I think the params being dxdfs, etc are a bit long. Consider shortening them to maybe ds for dialog settings, etc. I wrote a comment about it as well.
Tapping on the trackers blocked dialog while the typing animation is running dismisses the dialog instead of finishing the animation. This kills the default browser cta and probably got broken while doing the 2 buttons cta work.
There is a bug we talked about before where the instructions card is not aligned with the custom toast on the default browser page but we can ignore this until the experiment comes back with some results but we may want to keep a task to fix this in case we end up keeping that screen.
app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/statistics/pixels/Pixel.kt
Outdated
Show resolved
Hide resolved
app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt
Outdated
Show resolved
Hide resolved
app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt
Outdated
Show resolved
Hide resolved
app/src/androidTest/java/com/duckduckgo/app/cta/ui/CtaViewModelNextCtaTest.kt
Show resolved
Hide resolved
app/src/androidTest/java/com/duckduckgo/app/cta/ui/CtaViewModelTest.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt
Outdated
Show resolved
Hide resolved
…feature/cristian/cta_concept_test_2
* Making shorter the value of CTA pixel params * Update unit tests accordingly
…feature/cristian/cta_concept_test_2
|
Thanks for the review @marcosholgado
|
marcosholgado
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Thanks for the changes.
We discussed making another change for the dbo=e pixel to avoid a very edge case where the app gets backgrounded while the dialog is being shown and the user dismisses the dialog, in that case we receive dbo=e rather than dbo=dd but that can also happen with other edge cases and doesn't seem to be a clean solution when the user selects another browser as just once.
Feel free to merge once you figure out what you want to do with it, i.e remove dbo=e in the external browser just once scenario or any other solutions :)
Task/Issue URL: https://app.asana.com/0/488551667048375/1161278038072567/f
Tech Design URL:
CC:
Description:
In this experiment we are going to place our CTAs inside our Dax Journey. Our CTAs will match the style/tone of the existing Dax dialogs.
Default Browser CTA: After the user visits a site with trackers, we prompt a Dax dialog to tell the user that we blocked all the trackers in that site. After showing that dialog we will ask the user to set us as Default browser.
Search Widget CTA: When the user visits DuckDuckGo, a Dax dialog will appear informing our users that all their searches are private. After showing that dialog we will ask the user to add our search widget to protect their future searches.
Concept test variant mj: Vanila concept test
Concept test variant ml: Concept test with old ctas
Concept test variant mh: concept test with ctas as dax dialogs:
Repeat each of the following scenarios with previous steps:
a)
12. Click on "just once"
13. Ensure dialog appears again
14. Ensure no pixel is fired
14. Click on "always"
15. Ensure dialog dissapears
16. Ensure pixel "m_db_s" is fired with "dbo=d" as param
b)
12. Click on "just once"
13. Ensure dialog appears again
14. Click on "always"
15. Ensure dialog dissapears
16. Ensure pixel "m_db_ns" is fired with "dbo=jom" as param
c)
12. press back
13. Ensure dialog is dismissed
14. Ensure pixel "m_db_ns" is fired with "dbo=dd" as param
d)
12. select another browser and click "always"
13. Go back to DDG app
14. Ensure dialog is dismissed
14. Ensure pixel "m_db_ns" is fired with "dbo=e" as param
Repeat each of the following scenarios with previous steps:
a)
10. Click on "Go to settings"
11. Ensure pixel "m_odc_ok" is fired with "dbs" as param
12. Ensure user navigates to settings
13. Select ddg as default browser
14. Go back to the app
15. Ensure pixel "m_db_s" is fired with "fo=false, dbo=s" as param
16. Ensure cta is gone
b)
10. Click on "Maybe later"
11. Ensure pixel "m_odc_c" is fired with "dbs" as param
12. Ensure cta is gone
Internal references:
Software Engineering Expectations
Technical Design Template