Skip to content
This repository has been archived by the owner on Jul 29, 2022. It is now read-only.

refactor: Refactor navigation #743

Merged
merged 14 commits into from
Feb 5, 2020
Merged

refactor: Refactor navigation #743

merged 14 commits into from
Feb 5, 2020

Conversation

amaury1093
Copy link
Contributor

@amaury1093 amaury1093 commented Jan 28, 2020 •
edited
Loading

fixes #727
fixes #726
related #730

  • Remove :currentAccount in /accounts and /addresses routes
  • Refactor accounts-apps folder arch to have Accounts/ and Addresses/ subfolders
  • Made everything in Accounts and Addresses work
  • Refactor nested contexts

TODO:

@amaury1093 amaury1093 mentioned this pull request Jan 29, 2020
Merged
@amaury1093 amaury1093 changed the base branch from am-options to master January 30, 2020 14:04
@amaury1093 amaury1093 mentioned this pull request Jan 30, 2020
Closed
pmespresso
pmespresso reviewed Jan 31, 2020
View reviewed changes
Copy link
Contributor

@pmespresso pmespresso left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for me it's always stuck like this on electron as well as browser even though the Network tab shows it's getting messages:

edit: I was connecting on substrate --dev and indeed, it would have no peers.

packages/accounts-app/src/Accounts/AddAccount/Create/CreateAccount.tsx
@@ -160,7 +162,7 @@ export function Create(props: Props): React.ReactElement {

return (
<Stacked>
{keyringReady && <AddressSummary address={address} name={name} size='small' orientation='vertical' />}
{isKeyringReady && <AddressSummary address={address} name={name} size='small' orientation='vertical' />}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I'm pretty sure this KeyringContext is pretty much useless, or it should be according to the code, since ui-keyring calls cryptoWaitReady anyway. But if it fixes something, then let's keep it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to remove it and see (iirc it was not related to cryptoWaitReady but to keyring.loadall)

pmespresso
pmespresso suggested changes Jan 31, 2020
View reviewed changes
Copy link
Contributor

@pmespresso pmespresso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so i also don't see the keyring accounts/addresses listed as expected, even though the InputAddress seems to detect them:

Screenshot 2020-01-31 at 18 08 45

@pmespresso pmespresso merged commit 2a40745 into master Feb 5, 2020
@pmespresso pmespresso deleted the am-navigation branch February 5, 2020 10:34
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pmespresso pmespresso pmespresso approved these changes

Assignees
No one assigned
Labels
A0-pleasereview
Projects
None yet
Milestone
No milestone
2 participants
@amaury1093 @pmespresso