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

ui: rework app initialization #2385

Merged
merged 1 commit into from Jul 12, 2023
Merged

Conversation

buck54321
Copy link
Member

This reworks app initialization to achieve a number of ux goals.

  1. Adds chance for user to quickly configure native wallets and add
    known servers (view-only), client: allow skipping registration when adding a dex (view-only) #1986 (comment)
  2. Adds seed backup dialog
  3. Makes the Wallets view the landing page

image

Copy link
Contributor

@ukane-philemon ukane-philemon left a comment

Choose a reason for hiding this comment

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

Good work @buck54321. I will try testing this on my end.

@chappjc
Copy link
Member

chappjc commented Jun 14, 2023

Very nice. Wallets above servers would be better for the multi-coin wallet pivot, but this is great either way.

@JoeGruffins
Copy link
Member

Getting the same error as cli when trying to use:

$ ./dexc
failed creating web server: error loading localized html templates: error adding template init: template: init:35: comment ends before closing delimiter

@JoeGruffins
Copy link
Member

Looks good. A few nits:

Cannot click the actual check boxes here. Can press anywhere else but not on the check boxes themselves.
image

"skip this for now" looks like it should be clickable but clicking it does nothing. It seems you cannot skip looking at the seed at all.
image

After just making the client and navigating to markets you will see this. I think there is an extra loading screen when you go the login route that waits for markets to load and so we don't ever get here in the ui normally. This page mus be reloaded at some point to show the markets.
image

Probably in master, but cannot view token markets without making a token wallet. I can see the eth market already with no eth wallet yet so not sure why it is different. On loading candles indefinitely.
image

Copy link
Contributor

@martonp martonp left a comment

Choose a reason for hiding this comment

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

Looks good other than the issues which @JoeGruffins also noticed.

client/webserver/site/src/js/init.ts Show resolved Hide resolved
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

This is working very well for me with the last commit. Just some thoughts--

When you choose only to add wallets, no DEX host, we could add a link like this on the wallets page:

image

But that's not a new issue and not necessary. It could just be a plain <a href="/register">

The other thing, which is think is just a design decision, is when you add a DEX using this new interface, it's always "view-only" even if you had already had an account. We assume no account discovery by default is desirable. Should we? I restored from seed where I already had an account, and I saw:

image

And clicking discovered it so I got this simple form:

image

A slightly different approach that would avoid the need to add a "view-only" checkbox on this simplified init flow would be to first try assuming you want to discover an account, and then fall back to view-only if there was no known account. I don't think backend changes are needed for this.

Anyway, no need to make any changes. It works as-is! Thoughts? We can merge if there's no need to update in your mind.

@chappjc
Copy link
Member

chappjc commented Jul 10, 2023

Pls squash/rebase and we can merge.

This reworks app initialization to achieve a number of ux goals.

1) Adds chance for user to quickly configure native wallets and add
   known dexes (view-only)
2) Adds seed backup dialog
3) Makes the Wallets view the landing page
@chappjc
Copy link
Member

chappjc commented Jul 12, 2023

Rebased and squashed for you so we can merge this since it has been good with approvals for a while.
I'd still like to know what you think about the ideas here but any more changes can wait.

@chappjc chappjc merged commit 7112e23 into decred:master Jul 12, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants