Voting: use aragonAPI for React + React hooks #717
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2color
reviewed
Mar 19, 2019
2color
reviewed
Mar 19, 2019
2color
reviewed
Mar 19, 2019
2color
reviewed
Mar 19, 2019
sohkai
reviewed
Mar 22, 2019
AquiGorka
reviewed
Mar 25, 2019
AquiGorka
reviewed
Mar 25, 2019
AquiGorka
reviewed
Mar 25, 2019
AquiGorka
reviewed
Mar 27, 2019
AquiGorka
reviewed
Mar 27, 2019
AquiGorka
reviewed
Mar 27, 2019
sohkai
reviewed
Apr 7, 2019
There was a problem hiding this 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.
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()).
Since it contains more than a component.
sohkai
reviewed
Apr 9, 2019
| @@ -31,6 +26,8 @@ | |||
| "jsxBracketSameLine": false | |||
| } | |||
| ], | |||
| "react-hooks/rules-of-hooks": "error", | |||
- 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.
bpierre
commented
Apr 10, 2019
| prevVote.numData.votingPower === nextVote.numData.votingPower && | ||
| prevVote.numData.yea === nextVote.numData.yea && | ||
| prevVote.numData.nay === nextVote.numData.nay | ||
| ) |
There was a problem hiding this comment.
@sohkai now 1000x faster 😆
sohkai
approved these changes
Apr 10, 2019
ramilexe
pushed a commit
to ConsiderItDone/aragon-apps
that referenced
this pull request
Dec 9, 2021
The app is now using aragonAPI for React, and has also been a bit rearranged to take advantage of using React Hooks. The idea is to keep using the Voting app as an experiment to find and validate patterns related to React Hooks that we could use. A new file, `app-logic.js`, contains most of the logic to be used by `App.js`, by exporting `AppLogicProvider` and `useAppLogic()`. This is meant to be the pattern used by other apps in the future (providing that we validate this idea). Other changes: - VotingCard now skips any unnecessary update. - The vote texts (description and metadata) are now a separate component rather than being pre-rendered (that way it is easier to skip updates).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See aragon/aragon.js#259