[Wallet] Redesigning notification lists #1967

i1skn · 2019-11-29T16:02:41Z

Description

Redesign notification boxes on main mage
Redesign Incoming transaction list
~~Create a separate task to change currency in notifications to local~~ - https://github.com/celo-org/celo-monorepo/issues/2153
Unify implementation for Incoming, Outgoing and Escrow notifications.
Add Screenshots
Add Spanish translation

Tested

iOS & Android

Other changes

Some refactoring and little design tweaks

Related issues

Fixes Implement latest design for in-app notifications #1808

Backwards compatibility

Yes

Screenshots

Grouped notification

Notification list

Single notification

jmrossy

Overall really nice work! Please see comments below

packages/mobile/src/escrow/EscrowedPaymentLineItem.tsx

jmrossy · 2019-12-10T15:00:45Z

packages/mobile/src/home/NotificationBox.test.tsx

+    expect(getByText('outgoingPaymentRequests')).toBeTruthy()
+  })
+
+  it('renders outgoing payment request when they exist', () => {


How does this differ from the test above?

Single payment request vs multiple

Can we clarify the test description? I think the only diff is one character ;)

packages/mobile/src/navigator/Navigator.tsx

jmrossy · 2019-12-10T15:03:35Z

packages/mobile/src/notifications/NotificationList.tsx

+  )
+}
+
+export function titleWithBalanceNavigationOptions(title: string) {


this seems like an HOC / wrapper. Consider renaming to withTitleBalanceNavigationOptions

It's actually not, just a function.
Usage: Screen.navigationOptions = titleWithBalanceNavigationOptions(title)

Ah then maybe a get prefix would be helpful here

jmrossy · 2019-12-10T15:04:06Z

packages/mobile/src/notifications/NotificationList.tsx

+          <View style={[styles.scrollArea]}>{props.items.map(props.listItemRenderer)}</View>
+        </ScrollView>
+      ) : (
+        <Text style={[fontStyles.bodySecondary, styles.empty]}>{i18n.t('global:emptyList')}</Text>


Ditto feedback as above for use of i18n here

Also, is 'Empty List' all the design calls for here?

There is only one call to t, is that advisable to still use withNamespace?
Can you rephrase "is 'Empty List' all the design calls for here" - I did not get that :)

Yes please, still use withNamespace

Nevermind the Empty List bit for now. Was just a comment on the intended design. I think showing the words "Empty List" is suboptimal but fine for now

packages/react-components/components/BackButton.tsx

packages/react-components/components/BaseNotification.tsx

packages/react-components/styles/styles.ts

yarn.lock

jeanregisser

Awesome work on this 💯 🎉

jeanregisser · 2019-12-10T16:14:03Z

packages/mobile/src/escrow/EscrowedPaymentLineItem.tsx

  return (
    <Text numberOfLines={1} ellipsizeMode="middle" style={styles.oneLine}>
      <Text style={[fontStyles.subSmall]}>
-        {recipientPhone} - {message}
+        {recipientPhone ? recipientPhone : i18n.t('global:unknown')} {i18n.t('global:for')}
      </Text>
      <Text style={[fontStyles.subSmall, fontStyles.semiBold]}>
        {' '}


Concatenating strings like this makes it impossible to translate correctly. Some locale don't use the same ordering in words.
We could instead use the <Trans /> component and context.

Something along those lines:

<Trans i18nKey="escrowPaymentLine" tOptions={{ context: !recipientPhone ? 'missingRecipientPhone' : null }} > <Text numberOfLines={1} ellipsizeMode="middle" style={styles.oneLine}> <Text style={[fontStyles.subSmall]}>{{ recipientPhone }} for </Text> <Text style={[fontStyles.subSmall, fontStyles.semiBold]}>{{ moneyAmount }}</Text> </Text> </Trans>

With the following translation strings (not sure about the correctness of the indices I used though):

"escrowPaymentLine": "<1><2>{{recipientPhone}} for</2><3>{{moneyAmount}}</3></1>", "escrowPaymentLine_missingRecipientPhone": "<1><2>Unknown for</2><3>{{moneyAmount}}</3></1>",

thanks a lot! It does make a lot of sense! I remember you've mention something like this on one of our meetings.

packages/mobile/src/escrow/EscrowedPaymentListScreen.tsx

jeanregisser · 2019-12-10T16:26:43Z

packages/mobile/src/escrow/EscrowedPaymentReminderSummaryNotification.tsx

-      },
-    ]
+  itemRenderer = (item: EscrowedPayment, key: number) => {
+    return <EscrowedPaymentLineItem payment={item} key={key} />


Wondering, do we have some kind of ID we could use instead of the key?

What would be the benefit to use ID vs key?

I assumed the key here was an index in an array which should be avoided if possible:
https://reactjs.org/docs/lists-and-keys.html#keys

packages/mobile/src/paymentRequest/IncomingPaymentRequestListItem.tsx

packages/mobile/src/paymentRequest/OutgoingPaymentRequestListItem.tsx

packages/mobile/src/paymentRequest/PaymentRequestNotificationInner.tsx

packages/mobile/src/send/saga.ts

packages/mobile/src/web3/gas.ts

packages/react-components/styles/styles.ts

* New design * Group notification if there are more then 2 of the same type

codecov · 2019-12-11T16:07:15Z

Codecov Report

Merging #1967 into master will decrease coverage by 0.07%.
The diff coverage is 89.95%.

@@            Coverage Diff             @@
##           master    #1967      +/-   ##
==========================================
- Coverage   74.88%   74.81%   -0.08%     
==========================================
  Files         279      276       -3     
  Lines        7797     7766      -31     
  Branches      971      692     -279     
==========================================
- Hits         5839     5810      -29     
+ Misses       1842     1839       -3     
- Partials      116      117       +1

Flag	Coverage Δ
#mobile	`74.81% <89.95%> (-0.08%)`	⬇️

Impacted Files	Coverage Δ
...es/mobile/src/notifications/SimpleNotification.tsx	`100% <ø> (ø)`	⬆️
packages/mobile/src/escrow/saga.ts	`20.94% <ø> (ø)`	⬆️
packages/mobile/src/send/SendAmount.tsx	`77.07% <ø> (ø)`	⬆️
packages/mobile/test/values.ts	`100% <ø> (ø)`	⬆️
packages/mobile/src/send/saga.ts	`42.66% <0%> (ø)`	⬆️
...s/mobile/src/notifications/SummaryNotification.tsx	`100% <100%> (ø)`
packages/mobile/src/home/NotificationBox.tsx	`75.53% <100%> (+1.06%)`	⬆️
...bile/src/escrow/ReclaimPaymentConfirmationCard.tsx	`100% <100%> (ø)`	⬆️
...kages/mobile/src/transactions/TransferFeedIcon.tsx	`100% <100%> (ø)`	⬆️
packages/mobile/src/transactions/NoActivity.tsx	`96% <100%> (+0.16%)`	⬆️
... and 26 more

Continue to review full report at Codecov.

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

jmrossy

A few more small things but overall LGTM 👍

jmrossy · 2019-12-12T12:13:12Z

packages/mobile/src/escrow/EscrowedPaymentLineItem.tsx

@@ -28,3 +37,5 @@ const styles = StyleSheet.create({
    flexDirection: 'row',
  },
 })
+
+export default withNamespaces('inviteFlow11')(EscrowedPaymentLineItem)


Nit, use the typed namespace IDs: Namespaces.inviteFlow11

jmrossy · 2019-12-12T12:14:47Z

packages/mobile/src/escrow/EscrowedPaymentLineItem.tsx

+        }}
+      >
+        <Text style={fontStyles.subSmall}>{{ recipientPhone }} for </Text>
+        <Text style={[fontStyles.subSmall, fontStyles.semiBold]}>{{ amount }}</Text>


You don't have to fix this but just an FYI, we would generally prefer to have a style defined below which does something like this:

{ ...fontStyles.subSmall, ...fontStyles.semiBold }

But it's really nbd. Just an FYI on the convention we're roughly trying to keep to elsewhere

* master: [Wallet] Use Charles proxy to see eth JSON rpc calls when using forno (#2204) [Wallet] Disable skip button when the user enable contact access (#2224) [Wallet] Redesigning notification lists (#1967) [Wallet] Fix crash on iOS when segment is enabled (#2222) Update documentation wrt. epoch rewards fractions (#2182) Improvement facilitating to run a full node (#2130) [BlockchainApi] Add ability to get exchange rates from/to cGLD or cUSD (#2005) Improve reliability of e2e governance test (#2208) Fix/protocol test flakyness (#2155) Fix bignumber display in CLI (#2212) Doc changes to address frequently asked questions (#2209) Upgrade TS version (#2196) Wait for only waitTime - 1 blocks (#2207) Minor baklava docs reconnection fixes (#2215) Update walletkit gateway fee to fix transactions in forno mode (#2211) Update baklava network ID in docs for 1.1 (#2214) Support more than 1 attesation bot at a time (#2192) Check sync status of attestation service (#2191) Indicate to run Twilio globally (#2193) Add Twilio and attestation bot envs (#2194)

i1skn requested review from cmcewen and jmrossy as code owners November 29, 2019 16:02

i1skn force-pushed the notifications-redesign branch 3 times, most recently from e214f31 to 1458155 Compare December 9, 2019 15:57

i1skn force-pushed the notifications-redesign branch 2 times, most recently from 666aa20 to 461c083 Compare December 10, 2019 14:02

i1skn changed the title ~~🚧 WIP: [Wallet] Redesigning notification lists~~ [Wallet] Redesigning notification lists Dec 10, 2019

i1skn requested a review from a team December 10, 2019 14:23

jmrossy reviewed Dec 10, 2019

View reviewed changes

jmrossy requested a review from a team December 10, 2019 15:23

i1skn force-pushed the notifications-redesign branch 2 times, most recently from 332d13a to f4bba95 Compare December 10, 2019 16:08

jeanregisser approved these changes Dec 10, 2019

View reviewed changes

i1skn added 7 commits December 11, 2019 15:56

Implement new notifications on Home screen

c6e6dbd

* New design * Group notification if there are more then 2 of the same type

fix tests

cd055fa

Fix build

346eae3

Fix locales

b1f054d

Address the comments

02547a8

Address PR comments

f57b96b

Address comments in the PR

c80d4b7

i1skn force-pushed the notifications-redesign branch from e99eb14 to c80d4b7 Compare December 11, 2019 14:57

Revert for key in i18n

4d51962

i1skn added 3 commits December 11, 2019 23:55

Merge branch 'master' into notifications-redesign

7320831

Merge branch 'master' into notifications-redesign

09367aa

Fix ts-ignore for TS#3.7.3

7855ef9

jmrossy approved these changes Dec 12, 2019

View reviewed changes

jmrossy assigned i1skn Dec 12, 2019

Address comments

34096ab

i1skn added 4 commits December 12, 2019 13:29

Merge branch 'master' into notifications-redesign

253a53c

Merge branch 'master' into notifications-redesign

264fb3d

Merge branch 'master' into notifications-redesign

6384fd7

Merge branch 'master' into notifications-redesign

7aed2ef

i1skn merged commit 6e6543b into master Dec 12, 2019

i1skn deleted the notifications-redesign branch December 12, 2019 15:26

This was referenced Dec 13, 2019

Add ts-ignore to fix build #2254

Merged

Revert "Revert "Upgrade TS version"" #2255

Merged

snyk-bot mentioned this pull request Oct 14, 2020

[Snyk] Security upgrade mathjs from 5.10.3 to 7.5.1 denissnykio/celo-monorepo#284

Open

snyk-bot mentioned this pull request Sep 27, 2022

[Snyk] Fix for 18 vulnerabilities denissnykio/celo-monorepo#856

Open

snyk-bot mentioned this pull request Oct 4, 2022

[Snyk] Fix for 18 vulnerabilities denissnykio/celo-monorepo#862

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Wallet] Redesigning notification lists #1967

[Wallet] Redesigning notification lists #1967

i1skn commented Nov 29, 2019 •

edited

Loading

jmrossy left a comment

jmrossy Dec 10, 2019

i1skn Dec 10, 2019

jmrossy Dec 12, 2019

jmrossy Dec 10, 2019

i1skn Dec 10, 2019

jmrossy Dec 12, 2019

jmrossy Dec 10, 2019

jmrossy Dec 10, 2019

i1skn Dec 10, 2019

jmrossy Dec 12, 2019

jeanregisser left a comment

jeanregisser Dec 10, 2019

i1skn Dec 10, 2019

jeanregisser Dec 10, 2019

i1skn Dec 10, 2019

jeanregisser Dec 11, 2019

codecov bot commented Dec 11, 2019 •

edited

Loading

jmrossy left a comment

jmrossy Dec 12, 2019

jmrossy Dec 12, 2019

[Wallet] Redesigning notification lists #1967

[Wallet] Redesigning notification lists #1967

Conversation

i1skn commented Nov 29, 2019 • edited Loading

Description

Tested

Other changes

Related issues

Backwards compatibility

Screenshots

Grouped notification

Notification list

Single notification

jmrossy 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 11, 2019 • edited Loading

Codecov Report

jmrossy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i1skn commented Nov 29, 2019 •

edited

Loading

codecov bot commented Dec 11, 2019 •

edited

Loading