Improves Accounts & Balances Page #1160
Conversation
|
@isidorosp marking this PR "ready for review" again retriggered the CI runs. They now ran (but failed) |
Codecov Report
@@ Coverage Diff @@
## develop #1160 +/- ##
===========================================
- Coverage 71.72% 71.63% -0.09%
===========================================
Files 256 257 +1
Lines 15616 15684 +68
Branches 2201 2211 +10
===========================================
+ Hits 11200 11235 +35
- Misses 3828 3863 +35
+ Partials 588 586 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@kelsos This should be ready for review. Only thing left to do is after you merge your PR where you made the changes to how TabNavigation works I need to rebase on top to use the new structure. |
| }); | ||
| } | ||
|
|
||
| editBalance(balance: ManualBalance) { | ||
| this.$emit('editBalance', balance); | ||
| return balance; |
There was a problem hiding this comment.
Open to suggestions on a more elegant way to do it.
There was a problem hiding this comment.
What I am really confused about is why we need to return the balances at the click handler. I am under the impression that the @click is just a void function.
There was a problem hiding this comment.
I guess we don't. I think initially I had tried to do it with the Emit() decorator (which seems to need to return something) and this was just left over. Fixed.
There was a problem hiding this comment.
You don't really need to return something with emit unless you want to transform the data :)
There was a problem hiding this comment.
Should be fixed in latest commit
| } | ||
|
|
||
| @Watch('$route') | ||
| onRouteChange() { |
There was a problem hiding this comment.
As an alternative you could use the route guards for the same thing
https://router.vuejs.org/guide/advanced/navigation-guards.html#in-component-guards
There was a problem hiding this comment.
I think I will leave as-is for now unless you have a strong objection to this?
There was a problem hiding this comment.
no strong objection I can change it in the future 👍
| </v-select> | ||
| </v-col> | ||
| <v-col cols="2" class="hidden-sm-and-down"> | ||
| <v-tabs |
There was a problem hiding this comment.
This seems like a prime example of something to extract in a new component, or is it a mode of the same component?
There was a problem hiding this comment.
We could potentially split that tab/select mode into two different components and use this one to include the distinct components.
There was a problem hiding this comment.
I would say that it's a presentation+navigational component with two "modes" (wide mode and tall mode), but I think each implementation will probably differ enough that there may not be enough of a shared basis to abstract a common component out of it. Or did you mean to just make a specific sub-component only this page just to make it less crowded?
There was a problem hiding this comment.
I mean specific sub-components to make it less crowded :)
* Fix 1145 * Adds an Exchange Balances sub-page to Accounts & Balances where one can view asset balances by exchange. Clicking an exchange link in the dashboard now takes you here. The old views/ExchangeBalances component and associated route has been removed. * Splits Accounts & Balances into three pages corresonding to balance source (Exchanges / Blockchain / Manual) via tabbed navigation and adds corresponding routes * Moves Adding and Editing of balances/accounts to a common component using a wrapper (BigDialog). BigDialog is a large card that comes up from the bottom of the window and has more space than a normal dialog modal. * Touches up some UI elements in the asset details for blockchain balances (makes the buttons for "expand/contract" buttons that are circles, fixes gutters, right-aligns figures). * Fix 1154 Fixes tags filter behavior in manual balances * Fixes some CSS in the TagFilter so that the tag box doesn't fluctuate in height when you add/clear tags * Fix 1140 * Fixes sorting in the asset balances to use the label or address (if no label is present). * Blockchain addresses are now truncated. * Blockchain addressed are now blurred if privacyMode is enabled.
[ui tests]
* Fixes integration tests [ui tests]
@kelsos there's some ignores in here as I'm not sure what the right way to fix these lint errors is. Help needed!
* Adds a select instead of the vertical tabs in the exchangeBalance page within Accounts & Balances based on viewport width (responsive) [ui tests]
[ui tests]
* Adds some functionality to ensure that routes and props in exchangeBalances are properly shown when going back/forward through history * Cleans up the styling in the exchange selector [ui tests]
* Moves fiatBalances to manualBalances sub-page in Accounts & Balances (temporary until fiatbalances is deprecated).
* Fix so that Accounts & Balances is always selected in the navigation menu when we are on a sub-page * Fix linting [ui tests]
* Addresses review notes * Fixes integration tests [ui tests]
isidorosp
force-pushed
the
workon_1145_copy
branch
from
d2ef6e4
to
4f43743
Compare
* Addresses review notes * Rebase on updated TabNavigation PR * Updated Usage Guide to reflect new Accounts & Balances layout (and some edits for consistency throughout) * Updated changelog [ui tests]
|
@LefterisJP any feedback from your end on the changelog / user guide changes (or anything else ofc.)? |
There was a problem hiding this comment.
Very nice job @isidorosp! I really like the changes! It really looks better.
One thing we may want to note in the future is allow customization of the label | truncated address display for addresses. I want to see it but someone else may only want to see the label. But this can be done later.
| @@ -206,15 +206,18 @@ Rotki can import any trade CSV data exported from cointracking.info. But in gene | |||
| Tracking accounts and balances | |||
| ********************************** | |||
|
|
|||
| To track blockchain accounts&balances and also FIAT balances you need to visit the "Accounts/balances" section from the left sidebar. | |||
| To manage Accounts & Balances (Blockchain Balances, Exchange Balances, and Manual Balances including fiat) you need to visit the "Accounts & Balances" section from the left sidebar. | |||
There was a problem hiding this comment.
Perhaps add something here, either here or in its own subsection that shows you can also check exchange balances in that page.
There was a problem hiding this comment.
Good call! Added in next commit.
| @@ -2,6 +2,7 @@ | |||
| Changelog | |||
| ========= | |||
|
|
|||
| * :feature: `1160` The Accounts & Balances page layout has been updated to increase usability. It is now split across three sub-pages: Blockchain Balances, Exchange Balances, Manual Balances (includes Fiat Balances). Exchange Balances is a new page where you will be able to see all of your asset balances for each connected exchange (previously this was only accessible from the Dashboard by clicking on an exchange). | |||
| * :bug: `1155` Fix the cryptocompate price queries of LUNA Terra | |||
Yes, agreed! Initially we were just showing either label (if it existed) or address, and now I put both but w/ the truncated. Eventually there can be a setting like: Address rendering in-app (and we use a common component throughout): |
* Addresses Lefteris review notes * If no exchange has been show up, instead of an empty list shows a notification to set up an exchange connection * Tweaks TabNavigation CSS that was causing an unwanted effect in the Exchange Balance tabs [ui tests]
* Minor fixes so that all components using tabnavigation show up consistently (PremiumSettings was missing a v-container wrapper) * Fix linting
|
Failing test should be handled after rebase on develop and is due to #1184 |
Accounts & Balances page has gotten some love ❤️
The Accounts & Balances has now been split across three sub-pages: Blockchain Balances, Exchange Balances, Manual Balances (includes Fiat Balances).
To add a new balance of just click the "+" icon at the right of each sub-page header:
Exchange Balances is a new page where you will be able to see all of your asset balances for each connected exchange (previously this was only accessible from the Dashboard by clicking on an exchange). From now on clicking one of the Exchanges in the Dashboard will take you to this sub-page with the corresponding exchange selected.
The page is also responsive! The exchange selector changes from a vertical list to a select menu for those with smaller viewports.
Adding and editing balances/addresses/accounts* now uses a consistent component (BigDialog), which is also made to be friendly to those with smaller resolutions (and eventually mobile).
* Adding an Exchange and a Fiat Balance still uses the old form, as exchanges are managed through the "API Keys" page. This may change in the future. Fiat balances are basically edited directly in the form. Since this component will be deprecated eventually (Manual Balances has equivalent/improved functionality), it wasn't worth it to change this.
Full Changelog
Fix #1145
Fix #1154
Fix #1140
TODO: