Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Introduce a location overview page #4068

Merged
merged 1 commit into from
Feb 14, 2022
Merged

Introduce a location overview page #4068

merged 1 commit into from
Feb 14, 2022

Conversation

lukicenturi
Copy link
Contributor

Closes #4035

Checklist

  • The PR modified the frontend, and updated the user guide to reflect the changes.
  • Introduuce a location overview page

@lukicenturi lukicenturi force-pushed the 4035-introduce-a-location-overview-page branch 6 times, most recently from 30c8fe0 to a6a685c Compare February 10, 2022 09:09
@lukicenturi lukicenturi marked this pull request as ready for review February 10, 2022 09:11
@lukicenturi lukicenturi force-pushed the 4035-introduce-a-location-overview-page branch 7 times, most recently from 9952821 to 092ddc4 Compare February 11, 2022 09:39
kelsos
kelsos reviewed Feb 11, 2022
View reviewed changes
Copy link
Member

@kelsos kelsos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be easy to hide empty tables that are not loading?

@codecov
Copy link

codecov bot commented Feb 11, 2022 •
edited
Loading

Codecov Report

Merging #4068 (164d40c) into develop (5adffe0) will decrease coverage by 0.11%.
The diff coverage is 13.45%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #4068      +/-   ##
===========================================
- Coverage    55.59%   55.47%   -0.12%     
===========================================
  Files          737      741       +4     
  Lines        58137    58304     +167     
  Branches     10182    10221      +39     
===========================================
+ Hits         32320    32347      +27     
- Misses       23642    23775     +133     
- Partials      2175     2182       +7
Flag Coverage Δ
frontend_integration 51.09% <41.75%> (-0.10%) ⬇️
frontend_unit 15.79% <1.69%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
frontend/app/src/components/AssetBalances.vue 90.90% <ø> (ø)
...components/accounts/exchanges/ExchangeBalances.vue 0.00% <0.00%> (ø)
...ntend/app/src/components/assets/AssetLocations.vue 0.00% <0.00%> (ø)
frontend/app/src/components/helper/ListItem.vue 37.14% <0.00%> (ø)
...nd/app/src/components/locations/LocationAssets.vue 0.00% <0.00%> (ø)
.../app/src/components/locations/LocationValueRow.vue 0.00% <0.00%> (ø)
frontend/app/src/router/routes.ts 100.00% <ø> (ø)
frontend/app/src/store/balances/getters.ts 44.54% <0.00%> (-3.91%) ⬇️
frontend/app/src/views/Assets.vue 0.00% <0.00%> (ø)
frontend/app/src/views/LocationOverview.vue 0.00% <0.00%> (ø)
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5adffe0...164d40c. Read the comment docs.

kelsos
kelsos reviewed Feb 11, 2022
View reviewed changes
frontend/app/src/store/balances/getters.ts
Comment on lines +1035 to +1038
const currentExchangeRate = exchangeRate(mainCurrency);
const blockchainTotalConverted = currentExchangeRate
? blockchainTotal.multipliedBy(currentExchangeRate)
: blockchainTotal;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this check will work when there is no exchange rate for the selected currency, but which cases are these specifically? ETH/BTC?

Copy link
Contributor Author

@lukicenturi lukicenturi Feb 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure though. I saw that the currentExchangeRate can be undefined at manualBalanceByLocation getter, so I thought it is common to check it. Should I remove it? @kelsos

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no keep it, but I don't remember why the check is on manual balance, the only point I can think of this possibly being undefined is during the dashboard loading, but I am under the impression that we load the exchange rates early during the login.

@kelsos
Copy link
Member

kelsos commented Feb 11, 2022 •
edited
Loading

@lukicenturi along with the fixing of the lint issues please add a change log entry.

Other than that, if possible could we hide any empty tables that are not loading, and also the question remains, about which currencies under what conditions do not return an exchange rate.

Other than that the PR is good.

@lukicenturi lukicenturi force-pushed the 4035-introduce-a-location-overview-page branch 5 times, most recently from 1d5cfe2 to 5391f2a Compare February 12, 2022 07:20
@lukicenturi
Copy link
Contributor Author

lukicenturi commented Feb 12, 2022 •
edited
Loading

@lukicenturi along with the fixing of the lint issues please add a change log entry.

Other than that, if possible could we hide any empty tables that are not loading, and also the question remains, about which currencies under what conditions do not return an exchange rate.

Other than that the PR is good.

Changelog added, the empty tables are hidden.

@lukicenturi lukicenturi force-pushed the 4035-introduce-a-location-overview-page branch 3 times, most recently from f42fc16 to d637c65 Compare February 12, 2022 08:41
@lukicenturi lukicenturi force-pushed the 4035-introduce-a-location-overview-page branch from d637c65 to 164d40c Compare February 12, 2022 13:00
kelsos
kelsos approved these changes Feb 14, 2022
View reviewed changes
Copy link
Member

@kelsos kelsos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thank you

@kelsos kelsos merged commit c18e1dc into rotki:develop Feb 14, 2022
@lukicenturi lukicenturi deleted the 4035-introduce-a-location-overview-page branch February 19, 2022 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kelsos kelsos kelsos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduce a location overview page
2 participants
@lukicenturi @kelsos