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

Add table enhancements and owner validation #46

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

radchukd
Copy link
Contributor

@radchukd radchukd commented Oct 2, 2021

Closes #43, #44, #45

@radchukd
Copy link
Contributor Author

radchukd commented Oct 2, 2021

PR is a bit big, here's a summary:

1. Contexts
I added 2 contexts - OwnersContext and TransactionsContext, which provide data respectively and handle all the smart contract interactions, in result:

  • removed code repetition;
  • added single source of truth with easier access to it;
  • added separation between data and UI;
  • made loading more predictable;
  • made table manipulation possible.

2. Table
I added react-table library (MIT) to handle sorting and pagination in the transactions table.

3. Other
I changed the ReplaceOwnerModal according to #43.
I did everything in #44 (added jazzicon component, owner validation, renaming).

Copy link
Member

@apbendi apbendi left a comment

Choose a reason for hiding this comment

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

Hey @radchukd, looking great overall! A couple things I've noticed:

  • Sorting seems to be broken. Clicking the headers makes the table flicker, but the sorting order/column does not change
  • The "Show N" dropdown works well, but it should only show up if the wallet has more than 5 txs to display. Before that point, it should be hidden.

@apbendi
Copy link
Member

apbendi commented Oct 7, 2021

There also seems to be some kind of state bug occurring when navigating between wallets. Here's a brief example:

ogg-state-bug

Add txs count condition for txs count changing
Fix sorting table updates
Fix txs update from transtaction row
@radchukd
Copy link
Contributor Author

radchukd commented Oct 7, 2021

Now also closes #47

@apbendi
Copy link
Member

apbendi commented Oct 8, 2021

Hey @radchukd, sorting and threshold button changes looking good! However, I am still seeing the state issues in the UI when switching between wallets. Note how when I start with an empty wallet, switch to one with lots of txs, then navigate back to the empty wallet, I see empty tables displayed. When I refresh, it looks the way it should:

ogg-state-bug-more

@apbendi
Copy link
Member

apbendi commented Oct 8, 2021

@radchukd looks like there's a regression on the "Change Threshold" form validation. Not accepting anything other than 1, even when there are multiple owners:

change-threshold-bug

@radchukd
Copy link
Contributor Author

radchukd commented Oct 8, 2021

Oops, the problem was in a totally different place, sorry. I reverted the previous changes and pushed a new commit, should be good now.

@apbendi apbendi self-requested a review October 21, 2021 16:15
Copy link
Member

@apbendi apbendi left a comment

Choose a reason for hiding this comment

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

Hey @radchukd, the state issues I was seeing when navigating between wallets seems to be resolved, though I'm now seeing a similar issue when creating a new tx. Check it out:

new-tx-bug

@radchukd
Copy link
Contributor Author

@apbendi should be good now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change Owner Form Modifications
2 participants