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.

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

refactor: revamp how transactions are loaded on the dashboard #1103

Merged
merged 6 commits into from Feb 26, 2019

Conversation

@j-a-m-l
Copy link
Contributor

commented Feb 25, 2019

Proposed changes

This is the first PR of a series that would revamp the entire application to allow caching transactions, improving its performance and enhancing the code.

In this PR:

  • Allow trigger actions of the synchronizer directly
  • Changed the dashboard to not synch the contacts, now that they are not displayed (currently contacts are included as wallets, so this change wouldn't be effective until more changes)
  • The dashboard doesn't fetch transactions now. This makes the initial load of its transaction slower because the synchronizer fetches the wallet first, so there is a small delay. This would be addressed on the next PR,

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Refactoring (improve a current implementation without adding a new feature or fixing a bug)
  • Test (adding missing tests or fixing existing tests)

Checklist

  • I have read the CONTRIBUTING documentation
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
@ArkEcosystemBot

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

@alexbarnsley @luciorubeens - please review this in the next few days. Be sure to explicitly select labels so I know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

@j-a-m-l j-a-m-l changed the title refactor: revamp how transactions are loaded on the dashboard [WIP] refactor: revamp how transactions are loaded on the dashboard Feb 25, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Feb 25, 2019

Codecov Report

Merging #1103 into develop will decrease coverage by 0.03%.
The diff coverage is 77.77%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1103      +/-   ##
===========================================
- Coverage     40.2%   40.17%   -0.04%     
===========================================
  Files          207      207              
  Lines         5255     5237      -18     
  Branches      1033     1024       -9     
===========================================
- Hits          2113     2104       -9     
+ Misses        3026     3019       -7     
+ Partials       116      114       -2
Impacted Files Coverage Δ
src/renderer/pages/Wallet/WalletAll.vue 46.26% <ø> (ø) ⬆️
src/renderer/pages/Dashboard.vue 23.52% <ø> (-1.48%) ⬇️
src/renderer/services/synchronizer.js 20.31% <0%> (-0.33%) ⬇️
...s/Wallet/WalletTransactions/WalletTransactions.vue 17.27% <0%> (ø) ⬆️
src/renderer/store/modules/transaction.js 52.11% <100%> (+1.4%) ⬆️
src/renderer/services/synchronizer/wallets.js 66.66% <100%> (+0.43%) ⬆️
...rer/components/Dashboard/DashboardTransactions.vue 71.42% <100%> (-3.58%) ⬇️
src/renderer/components/Network/NetworkModal.vue 4.22% <0%> (ø) ⬆️
... and 2 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 0f0e460...7ab4580. Read the comment docs.

@j-a-m-l j-a-m-l changed the title [WIP] refactor: revamp how transactions are loaded on the dashboard refactor: revamp how transactions are loaded on the dashboard Feb 25, 2019

@luciorubeens
Copy link
Member

left a comment

Looks good so far.

@j-a-m-l j-a-m-l requested a review from luciorubeens Feb 26, 2019

@luciorubeens luciorubeens merged commit 6a92105 into develop Feb 26, 2019

1 check passed

ci/circleci: test-node-11 Your tests passed on CircleCI!
Details

@ArkEcosystemBot ArkEcosystemBot deleted the use-synchronizer-on-dashboard branch Feb 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.