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

Replace searchAccount action with withData HOC - Closes #2025 #2297

Merged
merged 13 commits into from Aug 1, 2019

Conversation

@slaweet
Copy link
Member

commented Jul 31, 2019

What issue have I solved?

#2025

How have I implemented/fixed it?

  • Update /src/components/transactions/explorerTransactions/explorerTransactions.js to use withData HOC instead of searchAccount action.
  • Update src/components/transactions/walletTransactions/walletTransactions.js to use withData HOC instead of searchAccount action.
  • Delete search.js actions and reducer.
  • Delete accountTransactions component and use explorerTransactions directly
  • Move fetching delegate info to delegateTab tab component with a new api util for this

How has this been tested?

Check the account page and wallet page works.

Review checklist

@slaweet slaweet self-assigned this Jul 31, 2019

@slaweet slaweet added this to Pull Requests in Version 1.20.0 via automation Jul 31, 2019

@slaweet slaweet force-pushed the 2025-replace-search-account-action branch 2 times, most recently from 36d3d6c to 97c87c8 Jul 31, 2019

slaweet added some commits Jul 31, 2019

@slaweet slaweet force-pushed the 2025-replace-search-account-action branch from 7193173 to 72c3412 Jul 31, 2019

slaweet added some commits Jul 31, 2019

@slaweet slaweet force-pushed the 2025-replace-search-account-action branch from 72c3412 to 001eed2 Jul 31, 2019

slaweet added some commits Aug 1, 2019

@slaweet slaweet marked this pull request as ready for review Aug 1, 2019

@slaweet slaweet requested a review from massao Aug 1, 2019

@massao
Copy link
Contributor

left a comment

🐛 The account page is not being updated correctly if you search for one account, and later search for a different one, the same happens if you click in a voted delegate through the account page.

It seems related to the withData autoload because it only runs on the componentDidMount, also found a problem with the getApiParams, since it sets them as default params for the request. if you use the ownProps.address for example, you have to explicit update the address when the props change. account.loadData({ address: this.props.address });

But since this wasn't introduced in this PR, not sure if should be fixed in this one. or create a ticket to fix in any place that this can be happening. What is your opinion @slaweet

@slaweet

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

@massao I researched a bit about the problem and found this solution: https://stackoverflow.com/a/54060331/2080716 implemented in f16a250
I think it will also solve problems we had on other pages. E.g. updating send or vote with launch protocol.

@slaweet slaweet requested a review from massao Aug 1, 2019

@massao

massao approved these changes Aug 1, 2019

Copy link
Contributor

left a comment

Nice job, thanks Vit!

@massao massao added the ready label Aug 1, 2019

@slaweet slaweet merged commit 4387a77 into development Aug 1, 2019

4 checks passed

Jenkins e2e tests e2e tests passed
Details
Jenkins test deployment Commit was deployed to test
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
coverage/coveralls Coverage remained the same at 94.765%
Details

Version 1.20.0 automation moved this from Pull Requests to Merged Pull Requests Aug 1, 2019

@slaweet slaweet deleted the 2025-replace-search-account-action branch Aug 1, 2019

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