[Wallet] Local currency v1.1 #1137
Conversation
- Added new setting - Removed global LOCAL_CURRENCY_SYMBOL
This is to be able to leverage its new currencies api
jeanregisser
force-pushed
the
jeanregisser/local-currency-v1.1
branch
from
903c73b
to
f91b8e6
Compare
Codecov Report
@@ Coverage Diff @@
## master #1137 +/- ##
========================================
+ Coverage 66.59% 66.7% +0.1%
========================================
Files 257 259 +2
Lines 7394 7454 +60
Branches 430 498 +68
========================================
+ Hits 4924 4972 +48
- Misses 2375 2384 +9
- Partials 95 98 +3
Continue to review full report at Codecov.
|
| import { headerWithCancelButton } from 'src/navigator/Headers' | ||
| import { navigateBack } from 'src/navigator/NavigationService' | ||
|
|
||
| const DEFAULT_CURRENCY_CODE = 'USD' |
There was a problem hiding this comment.
Nit, how about using your currency enum here instead of a string?
| throw new Error("Can't fetch local currency rate without a currency code") | ||
| } | ||
| const rate = yield call(fetchExchangeRate, localCurrencyCode) | ||
| yield put(fetchCurrentRateSuccess(localCurrencyCode, rate, Date.now())) | ||
| } catch (error) { | ||
| Logger.error(`${TAG}@fetchLocalCurrencyRateSaga`, error) |
There was a problem hiding this comment.
It would be good to yield put(showError... here with a custom message
There was a problem hiding this comment.
Since this is a background task, I think it would be a surprising experience to have an error banner displayed on unrelated screens.
I'm thinking also specifically about the case where the blockchain api endpoint is down and this error would trigger continuously until it succeeds. Pretty annoying for the user.
We would need additional logic to ensure we don't display the error too often.
In conclusion, I'd rather avoid that for now and keep it as is.
| @@ -8,7 +8,6 @@ interface Props { | |||
| word: string | |||
| selected: boolean | |||
| onSelectAnswer: (word: string, data: any) => void | |||
| key: string | |||
There was a problem hiding this comment.
Can you confirm this doesn't affect the other consumers of this component (the language selection screen and the backup phrase quiz)?
There was a problem hiding this comment.
I checked it doesn't affect other consumers of this component 👍
The key property is implicitly handled by React. The explicit type addition here wasn't necessary, I guess the intention was to force consumers of SelectionOption to pass it? Passing or not key depends on the usage. This PR showed a new use case which doesn't require it (FlatList items are already wrapped by keyed components).
|
Really nice work! Just a few small comments |
* master: (31 commits) Upgrade to Node 10 (#1148) [faucet] Add custom metrics (#1143) Add IdentityMetadata to Contractkit (#1126) [Wallet] Local currency v1.1 (#1137) Add attestation-service deploy (#1128) [Wallet] A few docs and build cleanup (#1138) [CircleCI]Add comment on how to fix lint checks (#1134) 2019-09-30 integration deployment (#1149) Update web3 provider to new integration url (#1151) [celotool]Add fast mode to celotool invite (#1135) Revert "Feature/909 proxy delegatecall" (#1146) Use contractkit in notification service (#1118) Feature/909 proxy delegatecall (#1003) integration deployment for 2019-09-29 (#1139) Add instructions for npm publication to tag commit (#1117) Client Logs Data Flow script update (#1055) Deploying latest proxy code in genesis (#1122) Enable floating promises check everywhere (fix issues) (#1115) [cli] Solution for build error contractkit on Linux 19.04 distro (#960) [Wallet] Merge back changes made for mx pilot (#1113) ... # Conflicts: # yarn.lock
* master: (35 commits) [Wallet] Network fee in transaction feed (#1145) New About Page Cover (#905) Upgrade to Node 10 (#1148) [faucet] Add custom metrics (#1143) Add IdentityMetadata to Contractkit (#1126) [Wallet] Local currency v1.1 (#1137) Add attestation-service deploy (#1128) [Wallet] A few docs and build cleanup (#1138) [CircleCI]Add comment on how to fix lint checks (#1134) 2019-09-30 integration deployment (#1149) Update web3 provider to new integration url (#1151) [celotool]Add fast mode to celotool invite (#1135) Revert "Feature/909 proxy delegatecall" (#1146) Use contractkit in notification service (#1118) Feature/909 proxy delegatecall (#1003) integration deployment for 2019-09-29 (#1139) Add instructions for npm publication to tag commit (#1117) Client Logs Data Flow script update (#1055) Deploying latest proxy code in genesis (#1122) Enable floating promises check everywhere (fix issues) (#1115) ... # Conflicts: # packages/web/src/about/About.tsx # packages/web/src/about/images/index.ts # packages/web/static/locales/en/about.json
Description
This PR improves support for local currencies:
Tested
Updated tests. Changes the new currency setting, observed the selected currency rate is fetched and the displayed amounts are updated.
Other changes
Related issues
Backwards compatibility
Yes.