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

Export labels with Finance export #835

Merged
merged 8 commits into from May 10, 2019

Conversation

Projects
None yet
4 participants
@AquiGorka
Copy link
Contributor

commented May 6, 2019

How to test:

# one terminal 
cd into `aragon/aragon`
git fetch
git rebase origin/master
npm i
REACT_APP_ASSET_BRIDGE=local npm run start

# another terminal
cd into `aragon/aragon-apps`
git fetch
git checkout feature/export-labels
npm i
npm run start
  • Open up browser to localhost:3000
  • Click on Finance and test out export button setting and removing local identities
  • Profit

AquiGorka added some commits May 6, 2019

Transfers: refactor to add labels to export
Included with this change:
- Refactor to React hooks with React.memo (feels much faster).
- It does not make sense to use React.useEffect to generate the
downloadable data as it would have to list as dependency every
addresses's local identity, so the data is now generated on demand when
the user clicks on download.
- Viewport refactor to useViewport

@AquiGorka AquiGorka requested review from bpierre and sohkai May 6, 2019

@sohkai
Copy link
Member

left a comment

❤️ A couple suggestions, since we might as well clean a few things up now that this has moved to hooks :)

Show resolved Hide resolved apps/finance/app/src/components/Transfers.js
Show resolved Hide resolved apps/finance/app/src/components/Transfers.js Outdated
Show resolved Hide resolved apps/finance/app/src/components/Transfers.js Outdated
Show resolved Hide resolved apps/finance/app/src/components/Transfers.js Outdated
Show resolved Hide resolved apps/finance/app/src/components/Transfers.js Outdated
Show resolved Hide resolved apps/finance/app/src/components/Transfers.js Outdated
Show resolved Hide resolved apps/finance/app/src/components/Transfers.js Outdated
Show resolved Hide resolved apps/finance/app/src/components/Transfers.js Outdated
Show resolved Hide resolved apps/finance/app/package.json
@coveralls

This comment has been minimized.

Copy link

commented May 6, 2019

Coverage Status

Coverage remained the same at 97.015% when pulling 97cc675 on feature/export-labels into 4b5094a on master.

@coveralls

This comment has been minimized.

Copy link

commented May 6, 2019

Coverage Status

Coverage remained the same at 97.015% when pulling 0274968 on feature/export-labels into 4b5094a on master.

@AquiGorka AquiGorka force-pushed the feature/export-labels branch from 18c709f to 55490ce May 6, 2019

@sohkai

sohkai approved these changes May 8, 2019

Show resolved Hide resolved apps/finance/app/src/components/Transfers.js Outdated
Show resolved Hide resolved apps/finance/app/src/components/Transfers.js

@AquiGorka AquiGorka merged commit 3b046c1 into master May 10, 2019

5 checks passed

License Compliance All checks passed.
Details
Travis CI - Branch Build Passed
Details
Travis CI - Pull Request Build Passed
Details
coverage/coveralls Coverage remained the same at 97.015%
Details
license/cla Contributor License Agreement is signed.
Details

@AquiGorka AquiGorka deleted the feature/export-labels branch May 10, 2019

@sohkai sohkai referenced this pull request May 13, 2019

Closed

0.7.2 #102

17 of 20 tasks complete
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.