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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Share identities via link #775

Merged
merged 35 commits into from May 16, 2019

Conversation

Projects
None yet
4 participants
@AquiGorka
Copy link
Contributor

commented May 9, 2019

Share local identities via link (and import them too 馃槃 )

Summary

  • When clicking on share the selected identities are "bundled" into a link (base64 encoded)
  • When the user opens up the link a new modal will show (when the wrapper instance exists and the labels have been parsed from the url)
  • The user can choose which items to save. This implementation will remove the user's existing local identities and override with the selected ones.

How to test

  1. Use the branch corresponding to this pull request git fetch && git checkout feature/share-identities
  2. Install the dependencies and start the server npm i && npm run
  3. Open http://localhost:3000/ in your browser and load an organization (rinkeby).

Pending

  • Animations
    • To show the Save modal
    • To hide the Save modal
    • To hide the Share modal when link has been copied (or not, decision needed)
    • To show the Preferences modal when saving (or maybe not, decision needed)
  • Loader when "saving" shared labels

@AquiGorka AquiGorka changed the base branch from master to feature/granular-export-labels May 9, 2019

@luisivan luisivan referenced this pull request May 9, 2019

Closed

Link for sharing labels #72

@AquiGorka AquiGorka force-pushed the feature/share-identities branch 2 times, most recently from b17f2e6 to 93e8bc0 May 9, 2019

@bpierre

This comment has been minimized.

Copy link
Member

commented May 10, 2019

@AquiGorka is it ready to be reviewed? Asking because of the 鈥減ending鈥 list.

@AquiGorka

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@bpierre after discussing with @dizzypaty about the animations (and how the Save labels modal is part of Preferences) I will make some big changes to this code (mostly on how and where to render) so the code that shares and imports will likely stay the same.
Having said all that, early reviews are always helpful.

@bpierre

This comment has been minimized.

Copy link
Member

commented May 10, 2019

@dizzypaty @AquiGorka I鈥檓 wondering if we shouldn鈥檛 make it more explicit that we are importing data coming from the outside on the importing screen? Maybe using the word 鈥渋mporting鈥 instead of 鈥渟aving鈥, or by displaying a warning message? Otherwise a click on the 鈥渟ave鈥 button might seem like a non-risky action, if the user assumes the data displayed is theirs.

@AquiGorka

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

"Save external custom labels"?

@AquiGorka AquiGorka force-pushed the feature/share-identities branch 2 times, most recently from 20bce07 to c5eb87a May 10, 2019

@AquiGorka

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@bpierre changes done, ready for review

@AquiGorka AquiGorka changed the base branch from feature/granular-export-labels to master May 13, 2019

@AquiGorka AquiGorka force-pushed the feature/share-identities branch 2 times, most recently from 8d8ff3d to 354dd62 May 13, 2019

@sohkai sohkai referenced this pull request May 13, 2019

Closed

0.7.2 #102

17 of 20 tasks complete
@dizzypaty

This comment has been minimized.

Copy link
Member

commented May 14, 2019

Loading screen when saving shared custom labels

LoadingRing component

WebApp-1366px - preferences-1@2x

WebApp-1366px - preferences-1@2x-1

@sohkai

This comment has been minimized.

Copy link
Member

commented May 15, 2019

@AquiGorka I've merged with master, noticed a few things:

  • The "Export" button is still clickable if no identities are selected, even though the cursor seems to indicate it's been deactivated
  • On importing, the "confirm removal" modal appears (this behaviour doesn't exist on master so might be due to some API differences during the merge)
@bpierre
Copy link
Member

left a comment

Looking good! I only have one important issue with unicode characters. Also, shouldn鈥檛 we have the same importation screen when we import from a file?

Show resolved Hide resolved src/components/Preferences/LocalIdentities.js Outdated
const handleDownload = useCallback(() => {
useEffect(() => {
if (shareModalOpen) {
setTimeout(() => inputRef.current.focus(), 0)

This comment has been minimized.

Copy link
@bpierre

bpierre May 15, 2019

Member

馃憣

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka May 16, 2019

Author Contributor

Thank you, got this gem from your code 馃槃

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

sohkai approved these changes May 15, 2019

Copy link
Member

left a comment

Looks good, confirmed the fixes :).

Left a number of clean up / API comments that we don't need to fix immediately for this release.

Show resolved Hide resolved src/components/Preferences/LocalIdentities.js
Show resolved Hide resolved src/components/Preferences/LocalIdentities.js Outdated
Show resolved Hide resolved src/components/Preferences/Preferences.js Outdated
Show resolved Hide resolved src/components/Preferences/Preferences.js
Show resolved Hide resolved src/components/Preferences/SharedLabels.js
Show resolved Hide resolved src/components/Preferences/SharedLabels.js
Show resolved Hide resolved src/hooks.js
Show resolved Hide resolved src/hooks.js
}
}, [])

return { isSharedLink, setIsSharedLink, sharedLabels, removeSharedLink }

This comment has been minimized.

Copy link
@sohkai

sohkai May 15, 2019

Member

Rather than directly exposing setIsSharedLink and removeSharedLink, it seems like we could roll them into a single API?

And could we detect isSharedLink from a non-null sharedLabels?

],
[selected]
)
return { selected, setSelected, allSelected, someSelected }

This comment has been minimized.

Copy link
@sohkai

sohkai May 15, 2019

Member

This also feels like we could include some of the toggling functionality (for toggling all and individual ids) to avoid repeating it.

Should we also use this directly in SelectableLocalIdentity?

@sohkai

sohkai approved these changes May 15, 2019

Copy link
Member

left a comment

Functionality all looks good to me, great job @AquiGorka! Let's work on the other review comments after this has been merged :).

AquiGorka and others added some commits May 16, 2019

SharedLabels: improve copy
Co-Authored-By: Brett Sun <qisheng.brett.sun@gmail.com>
SharedLabels: refactor code syntax
Co-Authored-By: Brett Sun <qisheng.brett.sun@gmail.com>

@AquiGorka AquiGorka force-pushed the feature/share-identities branch from 2ce17df to e1c47a4 May 16, 2019

AquiGorka added some commits May 16, 2019

@AquiGorka AquiGorka merged commit c7f2623 into master May 16, 2019

2 of 4 checks passed

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
install install
Details
license/cla Contributor License Agreement is signed.
Details

@AquiGorka AquiGorka deleted the feature/share-identities branch May 16, 2019

@AquiGorka AquiGorka referenced this pull request May 16, 2019

Open

Pending items Share custom labels via link #785

0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.