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

Voting: use aragonAPI for React + React hooks #717

Merged
merged 38 commits into from Apr 10, 2019

Conversation

Projects
None yet
5 participants
@bpierre
Copy link
Member

bpierre commented Mar 18, 2019

See aragon/aragon.js#259

@bpierre bpierre changed the title Voting react api Voting: use @aragon/api-react + React hooks Mar 18, 2019

Show resolved Hide resolved apps/voting/app/src/script.js Outdated
Show resolved Hide resolved apps/voting/app/package.json Outdated

@bpierre bpierre requested review from 2color and sohkai Mar 21, 2019

@bpierre bpierre marked this pull request as ready for review Mar 21, 2019

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 21, 2019

Coverage Status

Coverage remained the same at 96.734% when pulling 2af4569 on voting-react-api into c4c936f on master.

@bpierre bpierre requested a review from sohkai Apr 3, 2019

@sohkai
Copy link
Member

sohkai left a comment

I'm not sold on migrating App.js to use hooks completely.

I like it for simple components without too much logic, but for the main state container and logic controller of an app (which usually has no use for lifecycle hooks), it feels more difficult to read and understand due to everything being declared in-place without a lot of syntactic separation.

Particularly tricky is the dependencies lists, which looks likely to bite us at one point or another.

Show resolved Hide resolved apps/voting/app/src/app-hooks.js Outdated
Show resolved Hide resolved apps/voting/app/src/App.js Outdated
Show resolved Hide resolved apps/voting/app/src/components/VotePanelContent.js Outdated
Show resolved Hide resolved apps/voting/app/src/components/VotePanelContent.js Outdated
Show resolved Hide resolved apps/voting/app/src/App.js Outdated
Show resolved Hide resolved apps/voting/app/src/vote-hooks.js Outdated
Show resolved Hide resolved apps/voting/app/src/vote-hooks.js Outdated
Show resolved Hide resolved apps/voting/app/src/vote-hooks.js Outdated
Show resolved Hide resolved apps/voting/app/src/App.js Outdated
Show resolved Hide resolved apps/voting/app/src/App.js Outdated

bpierre added some commits Apr 8, 2019

Refactor App.js to move the logic into custom hooks
voting-app.js now contains most of the logic of the app.

Other changes:

- Remove vote-hooks.js: usePromise() is now in utils-hook.js, and the
  rest is in voting-app.js
- Remove provideNetwork (replaced by useNetwork() from @aragon/api-react)
- Replace the provideSettings HOC by a useSettings() hook.
- Refactor IdentityManager.js to expose a hook rather than a context.
- Remove app-hooks.js and app-contexts.js (made useless by specialized
  hooks / providers like SettingsProvider / useSettings() and
  IdentityProvider / useIdentity()).
Make IdentityManager.js a JS module
Since it contains more than a component.

@bpierre bpierre requested a review from sohkai Apr 9, 2019

bpierre added some commits Apr 9, 2019

@@ -31,6 +26,8 @@
"jsxBracketSameLine": false
}
],
"react-hooks/rules-of-hooks": "error",

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 9, 2019

Member

Nice!

Show resolved Hide resolved apps/voting/app/src/App.js
Show resolved Hide resolved apps/voting/app/src/App.js Outdated
Show resolved Hide resolved apps/voting/app/src/App.js Outdated
Show resolved Hide resolved apps/voting/app/src/vote-utils.js Outdated
Show resolved Hide resolved apps/voting/app/src/identity-manager.js Outdated
Show resolved Hide resolved apps/voting/app/src/identity-manager.js
Show resolved Hide resolved apps/voting/app/src/voting-app.js Outdated

bpierre added some commits Apr 9, 2019

Refactoring: voting text rendering
- Stop prerendering the vote texts (and only rerender them when needed).
- Move the vote-related hooks into vote-hooks.js (rather than app-logic.js).
- Move usePanelState() into utils-hooks.js.
prevVote.numData.votingPower === nextVote.numData.votingPower &&
prevVote.numData.yea === nextVote.numData.yea &&
prevVote.numData.nay === nextVote.numData.nay
)

This comment has been minimized.

Copy link
@bpierre

bpierre Apr 10, 2019

Author Member

@sohkai now 1000x faster 😆

@sohkai

sohkai approved these changes Apr 10, 2019

Copy link
Member

sohkai left a comment

One last small thing and I think this is good to go ⚡️!

sohkai added some commits Apr 10, 2019

@bpierre bpierre merged commit ad4a43c into master Apr 10, 2019

1 of 4 checks passed

License Compliance FOSSA is analyzing this commit
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
license/cla Contributor License Agreement is signed.
Details

@bpierre bpierre deleted the voting-react-api branch Apr 10, 2019

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