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

Granular export labels #768

Merged
merged 19 commits into from May 13, 2019

Conversation

Projects
None yet
5 participants
@AquiGorka
Copy link
Contributor

commented May 6, 2019

How to test:

cd into `aragon/aragon`
git fetch
git checkout feature/granular-export-labels
npm i
npm run start
  • Open up browser to localhost:3000
  • Click on Preferences and test out selectable custom labels for export
  • Profit

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

@luisivan luisivan referenced this pull request May 6, 2019

Closed

Granular label export #74

Show resolved Hide resolved src/components/Preferences/LocalIdentities.js Outdated
Show resolved Hide resolved src/components/Preferences/LocalIdentities.js Outdated
Show resolved Hide resolved src/components/Preferences/LocalIdentities.js Outdated
@2color

2color approved these changes May 7, 2019

Copy link
Contributor

left a comment

Relly nice addition. Code looks good to me.

Two things I noticed visually:

  • The export button's disabled state is not noticeable (the cursor changes though)
  • The first time the all checkbox is clicked after rendering, there's a weird re-render of the list. It doesn't happen on subsequent clicks.

Any thoughts on that?

@AquiGorka

This comment has been minimized.

Copy link
Contributor Author

commented May 7, 2019

The export button's disabled state is not noticeable (the cursor changes though)

You are right, it is on the to-do list: aragon/aragon-ui#154
I did make it so that it is not enabled even though it does not have any visual change.

The first time the all checkbox is clicked after rendering, there's a weird re-render of the list. It doesn't happen on subsequent clicks.

Let me do some research on this.

@AquiGorka

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

The first time the all checkbox is clicked after rendering, there's a weird re-render of the list. It doesn't happen on subsequent clicks.

Yes, this was happening and only in first render. It is fixed with 19803a8 but I had to change a lot on how local identities data is being handled to be able to use a WeakMap instead of a Map

@AquiGorka AquiGorka force-pushed the feature/granular-export-labels branch from 19803a8 to 5a25646 May 8, 2019

@AquiGorka AquiGorka force-pushed the feature/granular-export-labels branch from 5a25646 to 679ab9c May 8, 2019

@AquiGorka AquiGorka force-pushed the feature/granular-export-labels branch from 63540ff to 8c59e73 May 8, 2019

@sohkai

sohkai approved these changes May 8, 2019

Copy link
Member

left a comment

Looks and feels super good, just a few last things I noticed 🚢

Show resolved Hide resolved src/components/Preferences/LocalIdentities.js Outdated
return <EmptyLocalIdentities onImport={onImport} />
React.useEffect(() => {
setAddressesSelected(initialAddressesSelected)
}, [initialAddressesSelected])

This comment has been minimized.

Copy link
@sohkai

sohkai May 8, 2019

Member

Do we need this useEffect() if the useState() starts with initialAddressesSelected?

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka May 9, 2019

Author Contributor

Tried without it and it does not work on first render - which actually surprised me.

Show resolved Hide resolved src/components/Preferences/LocalIdentities.js

@AquiGorka AquiGorka force-pushed the feature/granular-export-labels branch from b126ff3 to c67b1c6 May 9, 2019

LocalIdentities: fix bug when exporting
Co-Authored-By: Pierre Bertet <hello@pierre.world>
@bpierre

bpierre approved these changes May 9, 2019

Copy link
Member

left a comment

Looking good! 🤸‍♂ I just left a few comments about minor things.

AquiGorka and others added some commits May 10, 2019

LocalIdentities: remove unnecessary label prop
Co-Authored-By: Pierre Bertet <hello@pierre.world>

@AquiGorka AquiGorka requested a review from bpierre May 13, 2019

@bpierre
Copy link
Member

left a comment

💯

@AquiGorka AquiGorka merged commit 66e25ae into master May 13, 2019

7 checks passed

License Compliance All checks passed.
Details
build build
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
install install
Details
license/cla Contributor License Agreement is signed.
Details
lint lint
Details

@sohkai sohkai referenced this pull request May 13, 2019

Closed

0.7.2 #102

17 of 20 tasks complete

@sohkai sohkai deleted the feature/granular-export-labels branch May 16, 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.