Skip to content

fix(DI-535): moved Infinite Discovery CTA up slightly - #13879

Merged
JanaeHijaz merged 1 commit into
mainfrom
janaehijaz/di-535
Aug 4, 2026
Merged

fix(DI-535): moved Infinite Discovery CTA up slightly#13879
JanaeHijaz merged 1 commit into
mainfrom
janaehijaz/di-535

Conversation

@JanaeHijaz

@JanaeHijaz JanaeHijaz commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

This PR resolves DI-535

Description

The CTA was too far down, and even worse for small screens/ large font sizes.

Before After

PR Checklist

  • I have tested my changes on the following platforms:
    • Android.
    • iOS.
  • I hid my changes behind a feature flag, or they don't need one.
  • I have included screenshots or videos at least on Android, or I have not changed the UI.
  • I have added tests, or my changes don't require any.
  • I added an app state migration, or my changes do not require one.
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

  • I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

  • bumped up the CTA

Need help with something? Have a look at our docs, or get in touch with us.

@JanaeHijaz JanaeHijaz self-assigned this Aug 4, 2026
@artsyit

artsyit commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

This PR contains the following changes:

  • Dev changes (bumped up the CTA - JanaeHijaz)

Generated by 🚫 dangerJS against 688676d

@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

🅰️ Single-pass review (baseline)

Summary

One-line spacing change: adds mb={4} to the CTA container in BrowsePromptStep so the "Start Swiping" button sits higher on screen. No logic, data, or API changes.

Issues Found

None blocking. Two things worth a look before merge.

🟢 Suggestion — the CTA is now inconsistent with the preceding step. src/app/Scenes/Onboarding/Screens/Onboarding/Components/BrowsePromptStep.tsx:36

<AnimatedFlex pb={`${bottom}px`} mb={4} gap={1} entering={enteringAnim}>

QuestionStep is the step immediately before this one (config.ts:11[STEP_ARTWORK_MONTAGE, STEP_WELCOME, STEP_QUESTION, STEP_BROWSE_PROMPT]), and its CTA sits flush at the bottom (QuestionStep.tsx:135-141: position: "absolute", bottom: 0 with pb set to the safe-area inset and no extra margin). Introduction.tsx:73-85 cross-fades the two steps in place, so the button will visibly jump up between "Continue" and "Start Swiping". If design asked for the higher position, it probably wants it on both steps.

🟢 Suggestion — pb and mb stack, so the gap varies by platform. Same line. Total space below the button is bottom + space(4): on most Android devices and hardware-button phones that is just space(4), on a notched iPhone it is ~34px more. That inset-dependent difference is the same class of problem this PR is fixing. A Math.max keeps the visual gap identical everywhere:

const space = useSpace()
// ...
<AnimatedFlex pb={`${Math.max(bottom, space(4))}px`} gap={1} entering={enteringAnim}>

useSpace() is already used widely in the codebase (e.g. src/app/Scenes/MyCollection/Screens/Insights/CareerHighlightBigCard.tsx:71).

Areas Reviewed

  • Architecture & Design: fine, no structural change.
  • Testing: Components/__tests__/BrowsePromptStep.tests.tsx covers render + onNext. A spacing prop does not need new coverage.
  • Security / Performance: not applicable.

Questions for Author

  1. Did you check this with a large font scale on a small device? That is the case the description calls out as worst, and the headline block is <Flex flex={1} justifyContent="center"> (line 25) — taking space away from it gives the wrapped text less room, so the fix trades CTA position against headline space in exactly that scenario.
  2. The before/after screenshots read as one platform. Since the change stacks with the safe-area inset, an Android capture would confirm the CTA lands where design wants it there too.

@claude

claude Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

🅱️ Two-pass review (candidate)

Summary

One line: adds mb={4} (40px) to the "Start Swiping" CTA container in BrowsePromptStep, lifting it off the bottom edge. The before/after screenshots in the description show the intended effect.

Issues Found

No blocking issues.

🟢 SuggestionBrowsePromptStep.tsx:36: gap={1} on this AnimatedFlex is a no-op, the container has a single child (Button). Since you are editing the line anyway, it could go.

Areas Reviewed

Architecture, security, performance, bugs: nothing notable. The change is a static style prop on a styled-system component and does not touch the animation, data, or navigation paths.

Testing: Components/__tests__/BrowsePromptStep.tests.tsx covers the rendered copy and onNext. Spacing is not worth asserting in a unit test, and the PR has visual evidence instead, so no test change needed here.

Questions for Author

  1. CTA alignment with the previous step. config.ts:11 puts STEP_QUESTION immediately before STEP_BROWSE_PROMPT, and Introduction.tsx:73-85 crossfades the two in place. QuestionStep.tsx:131-146 renders its "Continue" button at position: absolute; bottom: 0 with only the safe-area pb and no extra margin, so the two full-width CTAs now land 40px apart during the crossfade where they used to line up. The whole screen inverts (black to mono0, fillLight to fillDark) so it may read fine, but did design ask for the lift on this step only, or should QuestionStep move too?

  2. Which platform are the screenshots from? The lift stacks on the safe-area inset, so total bottom space is bottom + 40: about 74px on a device with a home indicator, about 40px where the inset is 0. Worth a glance on both if the 40px was picked against an iOS screen.

@JanaeHijaz
JanaeHijaz merged commit 7c1689d into main Aug 4, 2026
18 checks passed
@JanaeHijaz
JanaeHijaz deleted the janaehijaz/di-535 branch August 4, 2026 17:38
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.

2 participants