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

Wallet: initial wallet settings page #2572

Merged
merged 6 commits into from Jun 1, 2019

Conversation

Projects
None yet
3 participants
@Schmavery
Copy link
Contributor

commented May 31, 2019

  • General settings contains a list of wallets that lets you click into wallet-specific settings
  • Wallet names are editable -- I ran into a (good?) idea for handling empty names that I implemented for now
  • Had to deal with a graphql bug, submitted a PR to reason-apollo and found a workaround for now. lmk if you get any crashes.
  • uri-encoded the wallet pubkeys in the path to avoid weird errors when connected to the real node
  • Added "back" functionality and an active state to the settings button
  • Clicking the CODA logo brings you out of settings

Todo:

  • Lots of styling work
  • Hover states on a lot of things?
  • Copy public key, display staking, private key file path
  • Delete wallet modal

image
image

@Schmavery Schmavery requested review from bkase and figitaki May 31, 2019

Schmavery added some commits May 31, 2019

@o1pranay
Copy link
Contributor

left a comment

Looks good! Couple comments, mostly for my education 😛

<header className=Styles.header>
<div className=Styles.logo>
<div className=Styles.logo onClick={_ => ReasonReact.Router.push("/")}>

This comment has been minimized.

Copy link
@o1pranay

o1pranay May 31, 2019

Contributor

Nice 👍

{React.string("Wallet version: " ++ data##version)}
</span>
<div className=Styles.walletItemContainer>
{data##ownedWallets

This comment has been minimized.

Copy link
@o1pranay

o1pranay May 31, 2019

Contributor

curious - do we ever type the payload for requests? Eg. for SettingsQueryString, we know the shape of the JSON response, so we can make sure that a property that's not in there is not trying to be accessed. Would @bsRecord work for this?

This comment has been minimized.

Copy link
@Schmavery

Schmavery May 31, 2019

Author Contributor

All the payloads actually do get typechecked already, happy to explain more in person :)

[@react.component]
let make = (~publicKey) => {
let (addressBook, _) = React.useContext(AddressBookProvider.context);
let keyStr = PublicKey.toString(publicKey);

This comment has been minimized.

Copy link
@o1pranay

o1pranay May 31, 2019

Contributor

nit: can you just use key since you pass it in already key={PublicKey.toString(w##publicKey)}

This comment has been minimized.

Copy link
@Schmavery

Schmavery May 31, 2019

Author Contributor

The key field that's passed in is the react-specific key property for handling array diffing better, I'm not sure that it's great practice to use it and I don't think reason react would even let you

This comment has been minimized.

Copy link
@o1pranay

o1pranay May 31, 2019

Contributor

makes sense, sounds good

<div className=Styles.walletItemContainer>
{data##ownedWallets
|> Array.mapi(~f=(i, w) =>
<>

This comment has been minimized.

Copy link
@o1pranay

o1pranay May 31, 2019

Contributor

qq - is <> just short hand for div?

This comment has been minimized.

Copy link
@figitaki

figitaki May 31, 2019

Contributor

That's actually ReasonReact for Fragments (see here).

Task.liftPromise(() =>
mutation(~refetchQueries=[|"getWallets"|], ())
// TODO: Remove fetchPolicy="no-cache" after merge of
// https://github.com/apollographql/reason-apollo/pull/196

This comment has been minimized.

Copy link
@o1pranay

o1pranay May 31, 2019

Contributor

epic!

This comment has been minimized.

Copy link
@figitaki

figitaki May 31, 2019

Contributor

Haha intense writeup for a 1-line fix too.

</WalletQuery>
<div className=Styles.footer>
<Link onClick={_ => setModalState(_ => Some("My Wallet"))}>
{React.string("+ Add wallet")}

This comment has been minimized.

Copy link
@o1pranay

o1pranay May 31, 2019

Contributor

random idea - we might want to later use react-intl (or the equivalent reason library) and maybe have external contirbutors help us translate to different languages

This comment has been minimized.

Copy link
@figitaki

figitaki May 31, 2019

Contributor

Eventually yeah that'd be dope, but I don't think that's a priority yet.

This comment has been minimized.

Copy link
@Schmavery

Schmavery May 31, 2019

Author Contributor

We can search for React.string at that point and probably find most of these

This comment has been minimized.

Copy link
@o1pranay

o1pranay May 31, 2019

Contributor

cool, def agree not a priority rn

}}
</WalletQuery>
<div className=Styles.footer>
<Link onClick={_ => setModalState(_ => Some("My Wallet"))}>

This comment has been minimized.

Copy link
@o1pranay

o1pranay May 31, 2019

Contributor

One of the error states is trying to save wallet name that already exists - we may want to think about incrementing this based on # of wallets or something, so we don't default name them all "My wallet"

This comment has been minimized.

Copy link
@Schmavery

Schmavery May 31, 2019

Author Contributor

Yeah that makes sense
One thing to note is that nothing will break technically if you name 2 wallets the same thing -- idk if it's necessarily a priority to disable it

This comment has been minimized.

Copy link
@o1pranay

o1pranay May 31, 2019

Contributor

I'm just thinking we want to be defensive from a product standpoint if we're using an address book, because we don't want people confusing their wallets and transferring funds from the wrong one / forgetting which ones are staking / compressing

<div className=Styles.backHeader>
<span
className=Styles.backIcon
onClick={_ => ReasonReact.Router.push("/settings")}>

This comment has been minimized.

Copy link
@o1pranay

o1pranay May 31, 2019

Contributor

does ReasonReact.Router have a history.goBack() or pop() method?

This comment has been minimized.

Copy link
@Schmavery

Schmavery May 31, 2019

Author Contributor

We can do it if we need to yeah, didn't see any downside to doing it this way here for now

Schmavery and others added some commits Jun 1, 2019

@figitaki
Copy link
Contributor

left a comment

lgtm

@mergify mergify bot merged commit 661e2fc into master Jun 1, 2019

21 checks passed

Summary 1 rule matches
Details
ci/circleci: build-artifacts--testnet_postake Your tests passed on CircleCI!
Details
ci/circleci: build-artifacts--testnet_postake_many_proposers Your tests passed on CircleCI!
Details
ci/circleci: build-artifacts--testnet_postake_snarkless_fake_hash Your tests passed on CircleCI!
Details
ci/circleci: build-wallet Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test--fake_hash Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_bootstrap Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_catchup Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_delegation Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_five_even_snarkless Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_five_even_txns Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_holy_grail Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_snarkless Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_split Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_split_snarkless Your tests passed on CircleCI!
Details
ci/circleci: test--test_postake_txns Your tests passed on CircleCI!
Details
ci/circleci: test-unit--dev Your tests passed on CircleCI!
Details
ci/circleci: test-unit--test_postake_snarkless Your tests passed on CircleCI!
Details
ci/circleci: tracetool Your tests passed on CircleCI!
Details

@mergify mergify bot deleted the wallet/wallet-settings-draft branch Jun 1, 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.