Show loading text on incomplete exchange rate - #9
Conversation
| } | ||
| }) | ||
|
|
||
| const EXCHANGE_RATE_LOADING_TEXT = sprintf(strings.enUS['drawer_exchange_rate_loading']) |
There was a problem hiding this comment.
sprintf is not needed as there is no string replacement happening
| secondaryDisplayAmount: string, | ||
| cryptoCurrencyCode: string, | ||
| fiatSymbol: string, | ||
| fiatExchangeAmount: string, fiatCurrencyCode: string |
There was a problem hiding this comment.
fiatCurrencyCode should be on a newline
There was a problem hiding this comment.
Could have saved a lot of typing and made this routine clearer to read by making completeData accept separate parameters vs an object.
paullinator
left a comment
paullinator
left a comment
There was a problem hiding this comment.
Remove unnecessary use of sprintf and cleanup completeData and this is good to go
Sprintf not needed as the display text does not require interpolation Data validation moved out of the component to improve readability Tests added to exercise the data validation
| this.props.secondaryDisplayAmount === 0 | ||
| ? <Text style={styles.text}>Exchange Rate loading...</Text> | ||
| !this.completeData({secondaryDisplayAmount, cryptoCurrencyCode, fiatSymbol, fiatExchangeAmount, fiatCurrencyCode}) | ||
| ? <Text style={styles.text}> |
There was a problem hiding this comment.
@wolverineks Is this <Text> element the default from react-native? If so, then please use the FormattedText component instead so that it has correct font, etc.
Rest looks fine to me.
|
I approve the PR but it needs merge conflicts resolved |
3 participants
Move all loading text and logic into the ExchangeRate component
Display loading text when ExchangeRate has incomplete data
Localize text in exchange rate component