Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Wallet] Gold screen redesign #3764

Merged
merged 14 commits into from
May 20, 2020
Merged

[Wallet] Gold screen redesign #3764

merged 14 commits into from
May 20, 2020

Conversation

i1skn
Copy link
Contributor

@i1skn i1skn commented May 7, 2020

Description

This PR implements Celo Gold redesigned screens with some exceptions:

  • Fees brake-down is not implemented (first we need to change how we fetch fees form blockchain-api and de-hardcode other fees in Wallet codebase).
  • Sticky header with the Balance (currently it says just "Gold")

Other changes

  • Change Button logic with BtnSize.FULL a bit, so it's not needed to add manually styles every time we want a full width button.
  • Change Button logic, so it accepts string | Element (needed when we use inside the button text)
  • Small fixes after the upgrade of react navigation to v5

Tested

Android

Related issues

Backwards compatibility

Yes

Screenshots

@i1skn i1skn force-pushed the i1skn/gold-screen-redesign branch 4 times, most recently from 015c7e5 to 16f2765 Compare May 11, 2020 15:17
@i1skn i1skn mentioned this pull request May 12, 2020
50 tasks
@i1skn i1skn force-pushed the i1skn/gold-screen-redesign branch 6 times, most recently from 2cfecda to 4c6a287 Compare May 14, 2020 12:50
@codecov
Copy link

codecov bot commented May 14, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@cb4f663). Click here to learn what that means.
The diff coverage is n/a.

@i1skn i1skn force-pushed the i1skn/gold-screen-redesign branch 2 times, most recently from 5442940 to f67f9c4 Compare May 18, 2020 13:33
@@ -27,7 +27,7 @@ export enum BtnSizes {
export interface ButtonProps {
onPress: () => void
style?: StyleProp<ViewStyle>
text: string
text: string | JSX.Element
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed, when we show inside the button text.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -67,6 +67,12 @@ const fontStyles = StyleSheet.create({
center: {
textAlign: 'center',
},
mediumNumber: {
Copy link
Contributor Author

@i1skn i1skn May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not in the Design Kit, but we use it more than twice on Gold Screen - I figure we could use it the future. Cc @coreycelo

@i1skn i1skn force-pushed the i1skn/gold-screen-redesign branch 2 times, most recently from 18212d9 to 247fb45 Compare May 18, 2020 19:13
@i1skn i1skn requested a review from a team May 18, 2020 19:14
@i1skn i1skn marked this pull request as ready for review May 18, 2020 19:14
Copy link
Contributor

@jeanregisser jeanregisser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome!! 👍 🎉
I've suggested a couple of small changes.

Comment on lines 73 to 75
<SafeAreaView style={styles.container}>
<View style={styles.paddedContainer}>
<ScrollView>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For fullscreen views, it's nicer to put <SafeAreaView> inside the <ScrollView> so when scrolling up and down the content is not clipped before the bottom/top of the screen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would content overlap with the bar in the bottom on iPhone X in this case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but that's what is preferred visually, the difference is that there's some padding at the bottom so you can still fully view all the content from the scrollview without overlap.

Comment on lines 55 to 57
alignItems: 'center' as 'center',
},
headerTitleAlign: 'center',
headerTitleAlign: 'center' as 'center',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅

packages/mobile/src/navigator/Navigator.tsx Outdated Show resolved Hide resolved
@@ -27,7 +27,7 @@ export enum BtnSizes {
export interface ButtonProps {
onPress: () => void
style?: StyleProp<ViewStyle>
text: string
text: string | JSX.Element
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +130 to +136
function getStyleForWrapper(
size: BtnSizes | undefined,
style: StyleProp<ViewStyle>
): StyleProp<ViewStyle> {
return [{ flexDirection: size === BtnSizes.FULL ? 'column' : 'row' }, style]
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to be addressed in the PR but I think the current BtnSize API is limiting.
In another PR I needed a button having both FULL (full width) and SMALL (40pt height) behaviors.
I don't have a proposition right now but wanted to raise this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was thinking about this as well to split this into two verticals: font size and horizontal filling. And started to think that this is needed, when I saw your workaround on this, when you were needed full width with small font size.

Comment on lines 1 to 30
import colors from '@celo/react-components/styles/colors.v2'
import fontStyles from '@celo/react-components/styles/fonts.v2'
import * as React from 'react'
import { StyleSheet, Text, View } from 'react-native'

interface Props {
text: string
}

function SectionHead({ text }: Props) {
return (
<View style={style.container}>
<Text style={style.text}>{text}</Text>
</View>
)
}

const style = StyleSheet.create({
container: {
paddingTop: 20,
paddingBottom: 8,
paddingHorizontal: 16,
},
text: {
...fontStyles.h2,
color: colors.dark,
},
})

export default SectionHead
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated SectionHeadNew in a previous master that's already been merged:
https://github.com/celo-org/celo-monorepo/blob/ee5242cb59ef61a35718d76c8dc2447d0c71c7a0/packages/react-components/components/SectionHeadNew.tsx
We can use that one now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Gold tab they have a different styling, so I will rename to SectionHeadGold.tsx for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok, all good then 👍

Comment on lines +37 to +65
// TODO: find a way on how to show local exchangeRate without this hack
const exchangeRateAmount = {
value: localAmount.exchangeRate,
currencyCode: CURRENCIES[CURRENCY_ENUM.DOLLAR].code,
localAmount: {
value: localAmount.exchangeRate,
exchangeRate: localAmount.exchangeRate,
currencyCode: localAmount.currencyCode,
},
}

return (
<Touchable onPress={onPress}>
<View style={styles.container}>
<View style={styles.firstRow}>
<View style={styles.desc}>
<Text style={styles.txMode}>
{isSellGoldTx ? t('feedItemGoldSold') : t('feedItemGoldPurchased')}
</Text>
<>
<Text style={styles.exchangeRate}> @ </Text>
<CurrencyDisplay
amount={exchangeRateAmount}
hideSymbol={false}
hideCode={true}
showLocalAmount={true}
style={styles.exchangeRate}
/>
</>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're only interested in displaying a specific value and not doing conversions, it looks like you can simplify to this:

const localAmount = (isSellGoldTx ? makerAmount : takerAmount).localAmount!
const exchangeRateAmount = {
  value: localAmount.exchangeRate,
  currencyCode: localAmount.currencyCode,
  localAmount: null
}

// Then
<CurrencyDisplay
  amount={exchangeRateAmount}
  hideSymbol={false}
  hideCode={true}
  showLocalAmount={false}
  style={styles.exchangeRate}
/>

Copy link
Contributor Author

@i1skn i1skn May 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@i1skn i1skn May 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should decouple conversion and representation inside CurrencyDisplay, but that is for a different PR :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah 😢, I intended to merge LocalCurrency and CURRENCIES to avoid this kind of mess.
All good then 👍

@i1skn i1skn force-pushed the i1skn/gold-screen-redesign branch from d94fead to ad69b6e Compare May 20, 2020 13:21
@i1skn i1skn force-pushed the i1skn/gold-screen-redesign branch from ad69b6e to 79f9fca Compare May 20, 2020 14:17
Copy link
Contributor

@jeanregisser jeanregisser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@i1skn i1skn added the automerge Have PR merge automatically when checks pass label May 20, 2020
@mergify mergify bot merged commit d113e94 into master May 20, 2020
@mergify mergify bot deleted the i1skn/gold-screen-redesign branch May 20, 2020 14:56
@@ -59,9 +59,10 @@ export const emptyHeader: StackNavigationOptions = {
headerShown: true,
headerTitleStyle: [styles.headerTitle, componentStyles.screenHeader],
headerTitleContainerStyle: {
alignItems: 'center',
alignItems: 'center' as 'center',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw since this is now typed as StackNavigationOptions the as 'center' should be no longer needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Have PR merge automatically when checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Wallet Design: Gold
3 participants