-
Notifications
You must be signed in to change notification settings - Fork 277
Wallet List Sorting #5
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
Conversation
src/modules/Core/Account/settings.js
Outdated
|
|
||
| export async function setLocalLog (account, log) { | ||
| const logFile = getLocalLogFile(account) | ||
| let oldLogs = logFile.getText() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line dosn't really do anything and the next line won't wait on it. why is it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, let me get rid of that.
src/modules/Core/Account/api.js
Outdated
| }) | ||
|
|
||
| export const updateActiveWalletsOrderRequest = (account, activeWalletIds) => { | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will break standard.js. Did you run npm test before committing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a bit nuts to be using a standardization library that will break over something harmless like a blank line? I believe npm test or npm run test is built into git commit, so I guess it just didn't pick up on it?
src/modules/Core/Account/settings.js
Outdated
|
|
||
| export const getSyncedSubcategories = (account) => getSyncedSubcategoriesFile(account).getText() | ||
| .then((text) => { | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will also break standard.js
| return bitcoinPlugin | ||
| } | ||
|
|
||
| export const getLitecoinPlugin = (state: any) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you removed the special case of getLitecoinPlugin why not also remove getEtherumPlugin and getBitcoinPlugin and always use the more general getPlugin method?
| let symbol = findDenominationSymbol(walletData.denominations, currencyCode) | ||
| let multiplier, name, symbol | ||
| // const exchangeDenomination = SETTINGS_SELECTORS.getExchangeDenomination(state, data.currencyCode) | ||
| if (walletData.currencyCode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
multiplier, name and symbol will stay undefined if !walletData.currencyCode which can cause problems
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, this is just how I am checking as to whether this wallet has loaded or not. If those variables stay undefined then that means walletData.currencyCode should also be undefined and therefore the variables never get used / accessed and instead an ActivityIndicator (spinner) will get shown with no wallet information.
| {walletData.currencyCode? ( | ||
| <View style={[styles.rowContent]}> | ||
| <View style={[styles.rowNameTextWrap]}> | ||
| <T style={[styles.rowNameText]} numberOfLines={1}>{cutOffText(name, 34)}</T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how cutOffText works with undefined but if it can't handle it it will crash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this may sound strange but due to the react-native-sortable-listview package that we are using, I had to have one single component with a conditional checking whether the wallet object exists or not. walletData.currencyCode is being used as that flag. cutOffText will always receive a string because no wallet that has a currencyCode will ever not have a name.
The GUI elements that are shown if there is no walletData.currencyCode do not access any wallet variables and instead just show a basic spinner (ActivityIndicator).
| <T style={[styles.rowNameText]} numberOfLines={1}>{cutOffText(name, 34)}</T> | ||
| </View> | ||
| <View style={[styles.rowBalanceTextWrap]}> | ||
| <T style={[styles.rowBalanceAmountText]}>{truncateDecimals(bns.divf(walletData.primaryNativeBalance, multiplier).toString(), 6)}</T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again I don't know how bns.divf works with undefined but if it can't handle it it will crash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, no wallet object should ever not have a multiplier.
# This is the 1st commit message: Contract full copyable address with ellipsis (...) # This is the commit message #2: Internationalize strings for transactionList scene (sprintf) # This is the commit message #3: More i18n strings for transactionsList scene # This is the commit message #4: Slight adjustments to walletList i18n (sprintf) # This is the commit message #5: i18n text strings on settings overview scene # This is the commit message #6: i18n for settings and currency settings
The purpose of this change is to make the Wallet List show loading wallets (spinning activity indicator), and allow drag + drop sorting while in sort mode: