feat(points): Implement Points Home screen #5128

jophish · 2024-03-19T22:18:26Z

Description

For ACT-1022. Adds in the Points home screen and sets up Statsig to store points config information.

See the Statsig Dynamic Config here. Points visibility is gated by the feature gate here.

Note that at the moment, this PR is complete except for:

The actual Points logo/icon - I'm waiting on a high-quality vector image before adding it to the branding repo.
Some copy on the Swap bottom sheet.

I added in the functionality to automatically create "sections" on the home screen for different point values - we won't need this immediately, but it was kinda fun to add to figured why not :)

Test plan

Unit and manual tested. See video below.

points-home-new-2024-03-21_21.31.59.mp4

Related issues

Fixes ACT-1022.

Backwards compatibility

Yes.

Network scalability

If a new NetworkId and/or Network are added in the future, the changes in this PR will:

Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

src/statsig/constants.ts

src/points/PointsHome.tsx

locales/base/translation.json

src/components/PointsButton.tsx

emerge-tools · 2024-03-20T21:22:33Z

1 build increased size

Name	Version	Download	Change	Install	Change	Approval
Celo (test) org.celo.mobile.test	1.81.0 (146)	24.2 MB	⬆️ 9.5 kB (0.04%)	60.4 MB	⬆️ 20.5 kB (0.03%)	N/A

Celo (test) 1.81.0 (146)
org.celo.mobile.test

⚖️ Compare build
📦 Install build
⏱️ Analyze build performance

Total install size change: ⬆️ 20.5 kB (0.03%)
Total download size change: ⬆️ 9.5 kB (0.04%)

Largest size changes

Item	Install Size Change
main.jsbundle	⬆️ 20.5 kB

🛸 Powered by Emerge Tools

kathaypacific

looks good!! sorry i didn't quite understand the display logic but let's chat about it at our 1:1 later!

locales/base/translation.json

src/analytics/Events.tsx

src/points/PointsHome.tsx

src/points/ActivityCardSection.tsx

kathaypacific · 2024-03-21T11:04:54Z

src/points/activityCards/index.tsx

+    )
+  } else {
+    return (
+      <View testID={testID} style={styles.container}>


instead of a separate component per activity, could we have a config that contains all the customisable info and render everything together? i think this is in essence what you've done, but i'm surprised by how many layers and new files there are. naively i thought it'd be as straightforward as something like

const pointsConfig = [{ // some props, like image, name }, ...] ... render( <> ...some other stuff {pointsConfig.map(config => <ActivityCard {...config} />} </> )

the activity card would then take the name and decide on the stuff that is customised in a predictable way (e.g. we could have something like the bottomSheetTitle is always t(`points.activityCards.${activityName}.bottomSheet.title`)

We should prefer using static translation keys so we can eventually use static extraction.

https://valora-app.slack.com/archives/C025V1D6F3J/p1707472640807039

@jeanregisser What are the requirements for this? Does this mean we can't store a translation key in a variable and then pass that to the translation function? I understand that we shouldn't dynamically generate the translation key itself (i.e., they should be considered opaque) but can we safely do something like?

const translationKey = condition ? 'some.key' : 'some.other.key' ... t(translationKey)

yes almost, best is like this:

const translation = condition ? t('some.key') : t('some.other.key')

src/statsig/constants.ts

jeanregisser

Awesome! 🚀

locales/base/translation.json

src/icons/Celebration.tsx

src/points/ActivityCardSection.tsx

src/points/activityCards/index.tsx

src/points/types.ts

codecov · 2024-03-22T01:46:48Z

Codecov Report

Attention: Patch coverage is 96.68874% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 85.75%. Comparing base (98637c3) to head (4dcbbda).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5128      +/-   ##
==========================================
+ Coverage   85.70%   85.75%   +0.05%     
==========================================
  Files         730      739       +9     
  Lines       29862    30012     +150     
  Branches     5156     5178      +22     
==========================================
+ Hits        25592    25738     +146     
- Misses       4035     4038       +3     
- Partials      235      236       +1

Files	Coverage Δ
src/analytics/Events.tsx	`100.00% <100.00%> (ø)`
src/analytics/Properties.tsx	`100.00% <ø> (ø)`
src/home/WalletHome.tsx	`97.50% <100.00%> (+0.06%)`	⬆️
src/icons/Celebration.tsx	`100.00% <100.00%> (ø)`
src/icons/Checkmark.tsx	`100.00% <100.00%> (ø)`
src/icons/Rocket.tsx	`100.00% <100.00%> (ø)`
src/icons/SwapArrows.tsx	`100.00% <100.00%> (ø)`
src/navigator/Headers.tsx	`62.00% <100.00%> (+2.42%)`	⬆️
src/navigator/Screens.tsx	`100.00% <100.00%> (ø)`
src/points/ActivityCard.tsx	`100.00% <100.00%> (ø)`
... and 7 more

... and 1 file with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98637c3...4dcbbda. Read the comment docs.

…jophish/ret-1022

jophish · 2024-03-22T01:54:19Z

src/points/PointsHome.tsx

+    }
+  }
+  return (
+    <SafeAreaView style={styles.outerContainer}>


Fun little bug encountered during development for those knowledgeable/wanting to do some sleuthing - without the outerContainer style here, the page visually renders totally correctly, except is completely uninteractable (no scrolling, can't press on anything at all) under only the following circumstances:

Running on Android and

Page contents exceed screen size (meaning scrolling is required)

It all "just works" on iOS, and on Android when scrolling isn't required. Notably,_ removing either one of CustomHeader or BottomSheet made this issue go away... Satish and I spent some time trying to fix this, but still don't know why we were seeing this behavior 🤯

oh interesting.

Not directly related to that bug, but does the ScrollView extend to the bottom of the screen? If so the outer SafeAreaView should only apply to the top, and we should either add another SafeAreaView just for the bottom within the ScrollView, or adjust it's bottom padding using useSafeAreaInsets().
So it doesn't hit an invisible wall on devices with a home bar.

This is how it should behave toward the bottom:

having this safeareaview outside is fine since we've now got the custom header which shouldn't scroll :) to elaborate on Jean's comment, we should have edges={['top']} here and add a paddingBottom equivalent to the safe area bottom (or use another safe area view with the edges set to bottom only) on the scrollview contents to ensure that the end of the content isn't cut off.

weird android bug, i'm not sure i understand but perhaps you were experiencing this one?

…jophish/ret-1022

jeanregisser

Nice work! Already approved with some suggestions.

jeanregisser · 2024-03-22T08:08:45Z

src/points/types.ts

+import React from 'react'
+
+const pointsActivities = ['create-wallet', 'swap', 'more-coming'] as const
+export type PointsActivities = (typeof pointsActivities)[number]


Suggested change

export type PointsActivities = (typeof pointsActivities)[number]

export type PointsActivity = (typeof pointsActivities)[number]

jeanregisser · 2024-03-22T08:28:43Z

src/points/PointsHome.tsx

+    }
+  }
+  return (
+    <SafeAreaView style={styles.outerContainer}>


oh interesting.

Not directly related to that bug, but does the ScrollView extend to the bottom of the screen? If so the outer SafeAreaView should only apply to the top, and we should either add another SafeAreaView just for the bottom within the ScrollView, or adjust it's bottom padding using useSafeAreaInsets().
So it doesn't hit an invisible wall on devices with a home bar.

This is how it should behave toward the bottom:

jeanregisser · 2024-03-22T08:35:28Z

src/points/ActivityCard.tsx

+        </View>
+      )}
+      {cardDefinition.icon}
+      <Text style={styles.cardTitle}>{t(cardDefinition.title)}</Text>


Can we make this static?
i.e. moving it to cardDefinition directly?

... title: t('xxx') ...

kathaypacific

i have a few more small suggestions but don't feel like i need to look at the PR again! :)

kathaypacific · 2024-03-22T09:20:23Z

src/points/ActivityCard.tsx

+    opacity: isCompleted ? 0.5 : 1,
+  }
+  const cardContainer = cardDefinition.bottomSheet ? (
+    <Touchable


super nit: could we get away with using Touchable always, and if there is no bottom sheet then we make the Touchable disabled + pass undefined to the onPress? you could then move this directly into the render?

one other thing to test is when pressed on Android, does the touchable shadow stay within the bounds / corners of the card? i recently had to battle with this in SelectRecipientJumpstartButton

kathaypacific · 2024-03-22T09:24:51Z

src/points/ActivityCard.tsx

+    </>
+  )
+
+  const cardContainerStyle = {


super nit, also personal preference: i think reading react components is already pretty non linear and cyclical because of hooks and stuff, i think that we can help readability along a bit by keeping render blocks together as much as possible and only create new variables when we need. i think this component would be easier to read if we collapsed some of cardContainerStyle, cardContent, testID

kathaypacific · 2024-03-22T09:25:59Z

src/points/ActivityCard.tsx

+    alignItems: 'center',
+    padding: Spacing.Regular16,
+    backgroundColor: Colors.gray1,
+    height: 96,


just checking if we need to cap the height? this would crop content if font scaling on the device is enabled, i wonder if we should just let it scale freely and only cap the width? or if you are trying to force a minimum, maybe minHeight?

kathaypacific · 2024-03-22T12:29:50Z

src/points/PointsHome.tsx

+        testId={`PointsActivityBottomSheet`}
+      >
+        {bottomSheetParams && (
+          <View>


nit: maybe you don't need a view here, and can get away with a fragment<>?

kathaypacific · 2024-03-22T12:47:35Z

src/points/cardDefinitions.tsx

+    icon: <SwapArrows />,
+    defaultCompletionStatus: false,
+    bottomSheet: {
+      title: 'points.activityCards.swap.bottomSheet.title',


idk how serious we are about doing static extraction (we've been talking about it since i first joined and haven't really made a start on it, i'm also not sure it will work with OTA translations because once unused keys are removed, they might also be removed in Crowdin?) i saw @jeanregisser comment on this in another part of the PR, so we're not dynamically constructing the keys to keep the door for this open but i think the requirement is then also that we need to pass the t function in here?

kathaypacific · 2024-03-22T12:53:51Z

src/icons/CheckmarkWithCircleBorder.tsx

@@ -0,0 +1,18 @@
+import * as React from 'react'


i lost the original thread but i think you mentioned that we want to create a new icon here because the checkmark is not quite the same as the other checkmark 🫠 i have a couple feelings about this, one of my triggers is stuff that looks the same but slightly different because it's just not clear the value of maintaining extra code for the subtle differences. also subtle differences = inconsistency. i'd encourage us to be gatekeepers of consistency and question designers when we see something that is same same but different, most of the time we're able to keep using existing components (or refresh existing components) - and if the answer is "we absolutely must use this slightly different thing because of reasons XYZ" then at least we feel better about the extra code :)

for this could i request that we check with Kayla if we need to use a new checkmark? and if so, perhaps we can replace the existing checkmark with this one.

i also had a thought that perhaps for these icons they're intended only for the points use case, and maybe we can put the icons in the points folder...i'd be less annoying if this icon wasn't going in the global folder which is already hard to navigate

kathaypacific · 2024-03-22T12:57:26Z

src/points/PointsHome.tsx

+    }
+  }
+  return (
+    <SafeAreaView style={styles.outerContainer}>


having this safeareaview outside is fine since we've now got the custom header which shouldn't scroll :) to elaborate on Jean's comment, we should have edges={['top']} here and add a paddingBottom equivalent to the safe area bottom (or use another safe area view with the edges set to bottom only) on the scrollview contents to ensure that the end of the content isn't cut off.

weird android bug, i'm not sure i understand but perhaps you were experiencing this one?

MuckT · 2024-03-22T22:39:49Z

@jophish looks like the touchable area for the header info icon is overlapping with the others. We should wrap the icon similarly what's done in src/components/QrScanButton.tsx and include it along with others in ACT-1136. This seems fine as a follow up to me.

Read your comment.

(this method of accessing the points screen is temporary, just for testers atm)

### Description For [ACT-1022](https://linear.app/valora/issue/RET-1022/[wallet]-build-points-landing-screen-add-statsig-config). Adds in the Points home screen and sets up Statsig to store points config information. See the Statsig Dynamic Config [here](https://console.statsig.com/4plizaPmWwPL21ASV4QAO0/dynamic_configs/points_config). Points visibility is gated by the feature gate [here](https://console.statsig.com/4plizaPmWwPL21ASV4QAO0/gates/show_points). Note that at the moment, this PR is complete _except_ for: * The actual Points logo/icon - I'm waiting on a high-quality vector image before adding it to the branding repo. * Some copy on the Swap bottom sheet. I added in the functionality to automatically create "sections" on the home screen for different point values - we won't need this immediately, but it was kinda fun to add to figured why not :) ### Test plan Unit and manual tested. See video below. https://github.com/valora-inc/wallet/assets/569401/c854b3d3-31c8-4d54-92f2-e65ed6bc2b76 ### Related issues - Fixes [ACT-1022](https://linear.app/valora/issue/RET-1022/[wallet]-build-points-landing-screen-add-statsig-config). ### Backwards compatibility Yes. ### Network scalability If a new NetworkId and/or Network are added in the future, the changes in this PR will: - [X] Continue to work without code changes, OR trigger a compilation error (guaranteeing we find it when a new network is added)

jophish added 2 commits March 19, 2024 17:09

Initial commit

236bcfd

Implement points home screen

59586e7

jophish commented Mar 19, 2024

View reviewed changes

src/statsig/constants.ts Show resolved Hide resolved

jophish commented Mar 19, 2024

View reviewed changes

src/points/PointsHome.tsx Show resolved Hide resolved

jophish commented Mar 19, 2024

View reviewed changes

locales/base/translation.json Show resolved Hide resolved

jophish commented Mar 19, 2024

View reviewed changes

src/components/PointsButton.tsx Show resolved Hide resolved

jophish added 4 commits March 19, 2024 18:31

test id

911ce4d

Test

d9993dd

Merge

ecb1910

Stricter typing

d99ace1

kathaypacific reviewed Mar 21, 2024

View reviewed changes

jeanregisser reviewed Mar 21, 2024

View reviewed changes

jophish and others added 4 commits March 21, 2024 18:56

Testing

28b9b6c

Merge branch 'main' into jophish/ret-1022

5a08063

Fix styling issue

28968b0

Merge branch 'main' into jophish/ret-1022

408b6b2

jophish added 2 commits March 21, 2024 21:48

Use functional components for icons to appease knip

d3d4bf3

Merge branch 'jophish/ret-1022' of github.com:valora-inc/wallet into …

ccadba5

…jophish/ret-1022

jophish commented Mar 22, 2024

View reviewed changes

jophish and others added 5 commits March 21, 2024 21:58

Fix icon issue

d34853d

Merge branch 'main' into jophish/ret-1022

f775d18

Revert

7dc00af

Merge branch 'jophish/ret-1022' of github.com:valora-inc/wallet into …

c83b908

…jophish/ret-1022

nit

24d76c4

jeanregisser approved these changes Mar 22, 2024

View reviewed changes

kathaypacific approved these changes Mar 22, 2024

View reviewed changes

comments

a8668eb

Merge branch 'main' into jophish/ret-1022

4dcbbda

jophish added this pull request to the merge queue Mar 25, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 25, 2024

jophish added this pull request to the merge queue Mar 25, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 25, 2024

jophish added this pull request to the merge queue Mar 25, 2024

Merged via the queue into main with commit 451e866 Mar 26, 2024
16 checks passed

jophish deleted the jophish/ret-1022 branch March 26, 2024 00:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(points): Implement Points Home screen #5128

feat(points): Implement Points Home screen #5128

jophish commented Mar 19, 2024 •

edited

Loading

emerge-tools bot commented Mar 20, 2024 •

edited

Loading

kathaypacific left a comment

kathaypacific Mar 21, 2024

jeanregisser Mar 21, 2024 •

edited

Loading

jophish Mar 21, 2024 •

edited

Loading

jeanregisser Mar 22, 2024

jeanregisser left a comment

codecov bot commented Mar 22, 2024 •

edited

Loading

jophish Mar 22, 2024 •

edited

Loading

jeanregisser Mar 22, 2024

kathaypacific Mar 22, 2024

jeanregisser left a comment

jeanregisser Mar 22, 2024

jeanregisser Mar 22, 2024

jeanregisser Mar 22, 2024

kathaypacific left a comment

kathaypacific Mar 22, 2024 •

edited

Loading

kathaypacific Mar 22, 2024

kathaypacific Mar 22, 2024

kathaypacific Mar 22, 2024

kathaypacific Mar 22, 2024

kathaypacific Mar 22, 2024 •

edited

Loading

kathaypacific Mar 22, 2024

MuckT commented Mar 22, 2024 •

edited

Loading

	export type PointsActivities = (typeof pointsActivities)[number]
	export type PointsActivity = (typeof pointsActivities)[number]

feat(points): Implement Points Home screen #5128

feat(points): Implement Points Home screen #5128

Conversation

jophish commented Mar 19, 2024 • edited Loading

Description

Test plan

Related issues

Backwards compatibility

Network scalability

emerge-tools bot commented Mar 20, 2024 • edited Loading

1 build increased size

Celo (test) 1.81.0 (146)org.celo.mobile.test

kathaypacific left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanregisser Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

jophish Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanregisser left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 22, 2024 • edited Loading

Codecov Report

jophish Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanregisser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kathaypacific left a comment

Choose a reason for hiding this comment

kathaypacific Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kathaypacific Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MuckT commented Mar 22, 2024 • edited Loading

jophish commented Mar 19, 2024 •

edited

Loading

emerge-tools bot commented Mar 20, 2024 •

edited

Loading

Celo (test) 1.81.0 (146)
org.celo.mobile.test

jeanregisser Mar 21, 2024 •

edited

Loading

jophish Mar 21, 2024 •

edited

Loading

codecov bot commented Mar 22, 2024 •

edited

Loading

jophish Mar 22, 2024 •

edited

Loading

kathaypacific Mar 22, 2024 •

edited

Loading

kathaypacific Mar 22, 2024 •

edited

Loading

MuckT commented Mar 22, 2024 •

edited

Loading