Onboarding Brand Design Update: Update try-a-search dialog#8277
Onboarding Brand Design Update: Update try-a-search dialog#8277mikescamell merged 37 commits intodevelopfrom
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 838ca71. Configure here.
510fa21 to
d652b87
Compare
|
@LukasPaczos Draft ready for a look over on the approach when you have time 👍 |
d652b87 to
90c0968
Compare
LukasPaczos
left a comment
There was a problem hiding this comment.
Architecture Review: DaxBubbleCta and Brand Design CTAs
I've been looking at how DaxIntroSearchOptionsBrandDesignUpdateCta fits into the DaxBubbleCta hierarchy and I think there's an opportunity to set up the architecture more cleanly before the remaining brand design CTAs land.
The core issue
DaxIntroSearchOptionsBrandDesignUpdateCta extends DaxBubbleCta but overrides all 7 behavioral methods (showCta, clearDialog, setOnPrimaryCtaClicked, setOnSecondaryCtaClicked, setOnDismissCtaClicked, setOnOptionClicked, hideDaxBubbleCta). All 6 non-interface methods were changed from non-open to open specifically for this subclass. It inherits from DaxBubbleCta for sealed class membership and the pixel/data fields, but reuses none of the base behavior.
The old-layout subclasses (DaxIntroSearchOptionsCta, DaxIntroVisitSiteOptionsCta, DaxEndCta, DaxSubscriptionCta) override zero methods — they're pure data classes. The brand design CTA overrides all of them. This is a clean split that suggests a missing intermediate layer.
When the remaining 3 brand design CTAs arrive, each one would need to:
- Copy ~200 lines of animation/transition code from the existing brand design CTA
- Override the same 7 methods with mostly-identical implementations
- Keep the
allContentIncludeslist in sync across all brand design CTAs (needed for content-transition fade-outs)
Suggestion: extract BrandDesignBubbleCta as an intermediate abstract class
DaxBubbleCta (sealed)
├── DaxIntroSearchOptionsCta (old layout, 0 overrides)
├── DaxIntroVisitSiteOptionsCta (old layout, 0 overrides)
├── DaxEndCta (old layout, 0 overrides)
├── DaxSubscriptionCta (old layout, 0 overrides)
└── BrandDesignBubbleCta (abstract) (overrides 7 methods once)
├── DaxIntroSearchOptionsBrandDesignUpdateCta
├── DaxIntroVisitSiteOptionsBrandDesignUpdateCta (future)
├── DaxEndBrandDesignUpdateCta (future)
└── DaxSubscriptionBrandDesignUpdateCta (future)
The intermediate class owns the shared brand design infrastructure:
- Theme resolution (
resolveOnboardingContext) and dismiss button styling - The content-transition animation framework (fade-out old include, type title, fade-in new content)
clearDialog(),setOnDismissCtaClicked(),hideDaxBubbleCta()targeting brand design view IDs- The
allContentIncludeslist (one place to update as new content include types are added) - Default no-ops for
setOnPrimaryCtaClicked/setOnSecondaryCtaClicked
Concrete subclasses provide:
activeIncludeId: Int— which content include this CTA uses (e.g.R.id.optionsContent)configureContentViews(view: View)— populate the active include's children- Override
setOnOptionClicked/setOnPrimaryCtaClickedonly when that CTA type uses those controls
Each brand design CTA goes from ~250 lines (full override of everything) to ~30-50 lines of data + content configuration.
Secondary win: eliminate the isBrandDesignUpdate boolean threading
Right now, isBrandDesignUpdate is separately queried from ctaViewModel.isBrandDesignUpdateEnabled() and threaded through CtaViewState, Command.SetBrowserBackground, and BrowserTabFragment.showCta()/showDaxOnboardingBubbleCta() — even though the CTA's concrete type already encodes this information.
With BrandDesignBubbleCta as a type, all of these become is BrandDesignBubbleCta checks. The boolean, the view state field, and the command parameter all go away.
Things that naturally die when old CTAs are removed (no action needed now)
isModifiedControlOnboardingExperimentEnabledon the sealed class base — brand design CTAs never read it- The experiment-gated logic in
setOnOptionClicked(onboardingExperimentEnabled, configuration, ...)— brand design ignores both parameters - The
whenblocks inBrowserTabViewModelthat enumerate both old and brand design variants for the same behavior (e.g. lines 1299-1301) DaxBubbleCta's baseshowCta()implementation and the old-layoutclearDialog()
Clean collapse path
When old CTAs are eventually removed, DaxBubbleCta becomes a redundant wrapper around BrandDesignBubbleCta. The cleanup at that point is a straightforward inline: rename BrandDesignBubbleCta to DaxBubbleCta, drop the "BrandDesign" prefix from subclasses, delete dead base-class code. Building the intermediate class now sets this up as a trivial rename rather than a structural refactor.
Thanks for the review @LukasPaczos 🙌 Changes made, I agree this approach is better. Ready for round 2 🥊 |
d73c03b to
a32fc81
Compare
a32fc81 to
b2fe3e2
Compare
| android:layout_height="wrap_content" | ||
| android:textAppearance="@style/Typography.DuckDuckGo.Onboarding.Title.InContext" | ||
| android:visibility="invisible" | ||
| app:typography="h2" |
There was a problem hiding this comment.
title would not match the new onboarding titles, it's the old design system title, which would be 32sp and not 20sp, h2 is the closest match we have. This all needs to be addressed when the rebrand project kicks off and we get a proper new design system.
| android:clickable="true" | ||
| android:focusable="true" | ||
| android:textAppearance="@style/Typography.DuckDuckGo.Onboarding.Title.InContext" | ||
| app:typography="h2" /> |
There was a problem hiding this comment.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…serTabFragment and Cta
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…esignBubbleCta type checks Remove the isBrandDesignUpdate boolean threading from CtaViewState and SetBrowserBackground command, replacing with direct is BrandDesignBubbleCta type checks at the call sites. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…n CtaViewModelTest
If the user taps to skip during the initial fade-in animation, the withEndAction callback would still fire and re-type the title. Guard with animationFinished check so the callback is a no-op after skip. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Set title text directly when tap-to-skip fires before typing animation has started, preventing blank title when user taps during the initial fade-in window. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update tests to use DaxTryASearchBrandDesignUpdateBubbleCta (the extracted top-level class) instead of the old nested name. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…idth on tablets layout_width="match_parent" in ConstraintLayout ignores constraintWidth_max. Changed to 0dp so the 600dp max width is respected and the card centers on tablets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ove background maxHeight The dismiss button appeared grey during its alpha fade-in animation because the card's 7dp elevation shadow was visible through the semi-transparent button. Raised the button's translationZ from 1dp to 8dp (above the card's elevation) so it sits above the shadow layer, and set outlineProvider="none" to prevent the button itself from casting a shadow at the higher Z. Also removed the maxHeight constraint on the rebrand background ImageView and its associated dimen resources, matching the approach on the end-cta branch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Re-add the background max height dimension that was removed prematurely — 126dp for phones, 252dp for tablets (sw600dp). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…dialog (#8277) The card is already inside a NestedScrollView in the browser tab layout. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
#8277) Guard setBackgroundColor(0) behind a visibility check on the rebrand background view, so non-brand CTAs don't lose their default background. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ions (#8277) Android's resource parser strips unescaped double quotes. Add properly escaped versions in donottranslate.xml and reference them instead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
#8277) The fragment's hideDaxBubbleCta() already hides both the old and brand design dialog roots, making the CTA's polymorphic version redundant. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
#8277) Remove the ViewModel roundtrip for the brand design CTA background. The fragment already has the CTA in scope, so it sets the background directly. The existing onboarding path remains unchanged. Also removes the redundant hideDaxBubbleCta from the CTA class since the fragment's version already handles both old and brand design roots. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…8277) Import mobile.android.R as the default R and alias browser-api's R as BrowserR to reduce confusion about which module resources come from. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ansition (#8277) Alpha alone isn't a reliable signal — hideDaxBubbleCta sets the container GONE without resetting alpha, so after a DAX_INTRO reset the stale alpha caused showCta to take the content-transition path and skip showing the container. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Should be 16 and not 18 for in-context
4adb0eb to
4aeee32
Compare


Task/Issue URL: https://app.asana.com/1/137249556945/project/1207908166761516/task/1212699268790178?focus=true
Description
Add the brand design update version of the "try a search" onboarding dialog (DaxBubbleCta). This introduces:
DaxBubbleCtasubclass with brand design update background support and typing animationNestedScrollViewwrapper for the brand design dialog so it scrolls as a unit in landscape without restructuring the existing NTP layout hierarchySteps to test this PR
Designs
Apply patch
Note if you easily want to retrigger the dialog from closing the app for testing purposes you can apply this patch to hardcode showing the dialog
Brand design dialog in portrait
Brand design dialog in landscape (phone)
No regressions on NTP
Brand design dialog in portrait (tablet)
Brand design dialog in landscape (tablet)
UI changes
See task
Note
Medium Risk
Medium risk because it adds a new onboarding CTA flow with new view animations, theming/context wrapping, and changes how NTP backgrounds are shown/cleared via updated
SetBrowserBackgroundcommand handling.Overview
Adds a brand design update variant of the onboarding “try a search” bubble CTA, including new UI layout (scrollable container for landscape), option button styling, and a new typing-animation title view.
Updates CTA plumbing to conditionally serve the new CTA behind
OnboardingBrandDesignUpdateToggles, propagate the custom background resource, and adjustBrowserTabFragmentto switch between legacy vs rebrand background images and apply onboarding-themed background colors.Enhances
DaxOnboardingBubbleBrandDesignUpdateCardViewto support optional arrow rendering (showArrow), preserve/restore bottom margins when toggling the arrow, and resolve onboarding theme attributes via a fallback themed context.Reviewed by Cursor Bugbot for commit 4aeee32. Bugbot is set up for automated code reviews on this repo. Configure here.