Dax dialog with two buttons #713

cmonfortep · 2020-02-15T17:29:03Z

Task/Issue URL: https://app.asana.com/0/0/1162132608576977/f
Tech Design URL:
CC: https://app.asana.com/0/0/1158941144031452/f

Description:
This PR is some preparation work that will be related (and be used) in a future task, where we will implement new Ctas with two buttons.
For context: To show our CTAs as Dax dialogs we will make use of a Dax dialog with two buttons. Usually, the secondary button will be used to dismiss the dialog.
Design can be found here: https://app.asana.com/0/0/1158941144031452/f

Within the scope of this PR, you will find the following changes:

Refactor current DaxDialog to handle a new secondary button
Decoupling view-highlighting related logic from simple Dax dialogs into a more concrete dialog.
Changes in UI according to new design
Enable sending pixels when the secondary button is clicked
Small improvement to avoid showing a dark status bar when the dialog appears

Steps to test this PR:
Currently, the new Dax dialog with two buttons is not used in any part of the project (but it will be). Testing the new dialog with 2 buttons will be included on a future PR once we have the experiment ready to be reviewed.

Then, do we need to test something? yes, ensure new changes don't affect how normal dialogs are shown. To testing that part:

Hardcode variant mv
Navigate to BrowserActivity
Visit any site that triggers a Dax dialog
Ensure that dialog appears and it's rendered as in previous versions (except for commented changes in the description).
Example:

Possible triggers: a site with trackers, a site without trackers (wikipedia.org), a major entity (google).
To test different Ctas, you may need to perform a fresh install for each type, as showing some of them will disable showing others.

Internal references:

Software Engineering Expectations
Technical Design Template

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.

malmstein · 2020-02-17T08:07:02Z

app/src/main/res/layout/content_dax_dialog.xml

-            </androidx.constraintlayout.widget.ConstraintLayout>
+                <com.google.android.material.button.MaterialButton
+                    android:id="@+id/secondaryCta"
+                    style="@style/Widget.AppCompat.Button.Borderless.Colored"


does it make sense to extract this to our own Button style?

malmstein · 2020-02-17T08:09:19Z

app/src/main/java/com/duckduckgo/app/cta/ui/CtaViewModel.kt


+    fun onUserClickCtaSecondaryButton(cta: SecondaryButtonCta) {
+        cta.secondaryButtonPixel?.let {
+            pixel.fire(it, cta.pixelSecondaryButtonParameters())


is this ok being launched in the main thread?

ApiBasedPixel:Pixel will excecute it in a background thread. So we are good here.

malmstein · 2020-02-17T08:11:03Z

If this is not going to be used for now, does it make sense to merge into develop or to keep a feature branch where you add multiple PRs? Just wondering if something were to change before it's finished, we'd have to change code that is potentially released to Play Store but is not being used yet.

cmonfortep · 2020-02-17T09:24:05Z

Thanks @malmstein for the review. About target branch on this PR, I'm happy with both approaches, I think both are valid but I will stick to the preferred one for the team. If it feels more natural creating a feature branch and merge everything there, I will move to that one and we can discuss this topic on our next mobile functional meeting.

marcosholgado · 2020-02-17T11:00:13Z

Assigning to myself to test all the cta scenarios since I know them very well but @malmstein and others, feel free to do your reviews as well, I just want to make sure all the ctas still work as expected :)

subsymbolic · 2020-02-17T13:59:13Z

@cmonfortep yes, please don't merge items not ready for release into develop.

marcosholgado

LGTM, left a comment in case you want to think about naming :D

marcosholgado · 2020-02-17T16:37:38Z

app/src/main/java/com/duckduckgo/app/global/view/DaxDialog.kt

+    fun setDaxText(daxText: String)
+    fun setButtonText(buttonText: String)
+    fun setDialogAndStartAnimation()
+    fun onAnimationFinishedListener(onAnimationFinished: () -> Unit)


The onAnimationFinishedListener is specific to the typing animation and is triggered when it finishes. I would suggest to move it to another interface or make it specific to the TypewriteDaxDialog class but this would make DaxDialogHighlightView a bit messier.
The other option is to assume the DaxDialog always is going to have the typing animation (which it does for now) and maybe rename TypewriteDaxDialog to something more generic? Having TypewriteDaxDialog makes me think there is another version without the typing animation when in reality there isn't.
I don't think is massively important to stop the PR but it feels a very specific name for something that's always used like that.

Actually, onAnimationFinishedListener can be moved into an interface and DaxDialogHighlightViewwill not be affected. The only part that will be affected is where we create those dialogs. But I liked that you commented on that. I think we can delay that change till we introduce a different Dax dialog, that is also the reason why I didn't segregate that interface into different ones.
As per the name, I'm fine changing the name or using this concrete one (because the interface takes the generic name), so I'm open to suggestions here. Tbh, I could come up with something else.

* Dax dialog with two buttons (#713) * New variant: Showing DefaultBrowserCta and SearchWidgetCta as Dax dialogs (#714) * Insert DefaultBrowser Cta and SearchWidget Cta as Dax dialog during Dax Journey (#716) * New control group and changes on when to show search widget cta (#723)

cmonfortep added 6 commits February 15, 2020 16:25

Theming new values to be used in dax dialog secondary button

afc6f48

Daxdialog layout changes to simplify required design.

927d345

Removing one layer of ConstraintLayout for one LinearLayout to simplify working with margins between buttons.

Send pixels when secondary button is clicked using secondaryPixelName…

704af49

… from SecondaryButtonCta

When user clicks on secondary button, notify CtaViewModel

d9d5b3d

* Detach highlighting logic from normal DaxDialogs.

b847031

* Introduce new logic to show/hide a secondaryButton on DaxDialogs. * Change DaxDialog creation logic inside Cta. * Adapt BrowserTabFragment listener logic to new chanes.

Avoid showing a dark statusbar when dialog is shown

5227754

cmonfortep requested review from CDRussell, marcosholgado and subsymbolic as code owners February 15, 2020 17:29

malmstein reviewed Feb 17, 2020

View reviewed changes

cmonfortep added 2 commits February 17, 2020 10:22

Style created for each Dax dialog button.

afbcba2

Format file.

2ee17bc

cmonfortep requested a review from malmstein February 17, 2020 09:25

cmonfortep changed the base branch from develop to feature/cristian/fb_insert_cta_concept_test_exp2 February 17, 2020 09:36

marcosholgado self-assigned this Feb 17, 2020

fix: statusbar showing random color in high api devices

4c5a1bc

marcosholgado approved these changes Feb 17, 2020

View reviewed changes

cmonfortep merged commit a5f9839 into feature/cristian/fb_insert_cta_concept_test_exp2 Feb 19, 2020

cmonfortep deleted the feature/cristian/two_buttons_dialog branch February 19, 2020 13:03

cmonfortep mentioned this pull request Feb 27, 2020

Insert CTAs on concept test Experiment#2 #724

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Dax dialog with two buttons #713

Dax dialog with two buttons #713

Uh oh!

cmonfortep commented Feb 15, 2020 •

edited

Loading

Uh oh!

malmstein Feb 17, 2020

Uh oh!

malmstein Feb 17, 2020

Uh oh!

cmonfortep Feb 17, 2020

Uh oh!

malmstein commented Feb 17, 2020

Uh oh!

cmonfortep commented Feb 17, 2020 •

edited

Loading

Uh oh!

marcosholgado commented Feb 17, 2020

Uh oh!

subsymbolic commented Feb 17, 2020

Uh oh!

marcosholgado left a comment

Uh oh!

marcosholgado Feb 17, 2020

Uh oh!

cmonfortep Feb 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Dax dialog with two buttons #713

Dax dialog with two buttons #713

Uh oh!

Conversation

cmonfortep commented Feb 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Internal references:

Uh oh!

malmstein Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

malmstein Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

cmonfortep Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

malmstein commented Feb 17, 2020

Uh oh!

cmonfortep commented Feb 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcosholgado commented Feb 17, 2020

Uh oh!

subsymbolic commented Feb 17, 2020

Uh oh!

marcosholgado left a comment

Choose a reason for hiding this comment

Uh oh!

marcosholgado Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

cmonfortep Feb 17, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cmonfortep commented Feb 15, 2020 •

edited

Loading

cmonfortep commented Feb 17, 2020 •

edited

Loading