Conversation
|
CLA Assistant Lite All Contributors have signed the CLA. |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
|
Travis automatic deployment: |
1 similar comment
|
Travis automatic deployment: |
fernandomg
left a comment
fernandomg
left a comment
There was a problem hiding this comment.
There's this function sameAddress that what it does is just string comparison. Can we use it instead of making the conversion explicit in every place?
And thinking forward, a rename of the function would be great or create a different function sameString and make sameAddress return the result of calling sameString(this is to preserve sameAddress, and if for any reason in the future we need to extend the way we compare addresses).
(ok, that was unnecessarily long 🤷♂️)
WDY?
| let rate = 0 | ||
|
|
||
| if (targetCurrencyValue === AVAILABLE_CURRENCIES.ETH) { | ||
| if (targetCurrencyValue === nativeCoin.symbol.toUpperCase()) { |
There was a problem hiding this comment.
given all the other cases, shouldn't it be toLocaleUpperCase() instead?
| //export type AVAILABLE_CURRENCIES_KEYS = keyof typeof AVAILABLE_CURRENCIES | ||
| //export type AVAILABLE_CURRENCIES_VALUES = typeof AVAILABLE_CURRENCIES[AVAILABLE_CURRENCIES_KEYS] |
There was a problem hiding this comment.
Can we get rid of the commented code?
|
Travis automatic deployment: |
| * @returns {boolean} | ||
| */ | ||
| export const sameString = (str1: string | undefined, str2: string | undefined): boolean => { | ||
| if (!str1 || !str2) { |
There was a problem hiding this comment.
what if both are empty string? 😬 or it's a case intentionally left out?
There was a problem hiding this comment.
sameAddress has the same approach. Well, maybe it's better that way to not break anything else
There was a problem hiding this comment.
yes.. they are the same string literal, it would be incorrect to return false IMHO.
| let rate = 0 | ||
|
|
||
| if (targetCurrencyValue === AVAILABLE_CURRENCIES.ETH) { | ||
| if (sameString(targetCurrencyValue, nativeCoin.symbol)) { |
There was a problem hiding this comment.
Do we need to import nativeCoin here? Shouldn't we still use AVAILABLE_CURRENCIES?
As I can see you added the network token here.
Should we rename it to AVAILABLE_CURRENCIES.NETWORK_TOKEN?
|
Travis automatic deployment: |
|
Travis automatic deployment: |
1 similar comment
|
Travis automatic deployment: |
|
I had to deploy this in my local env because the PR is automatically set for the Rinkeby network In the Rinkeby network itself shows the ETH default value Looks good to me |
|
Travis automatic deployment: |
5 participants
Closes #1498.
src/assets/iconstosrc/config/assets.