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

site: Notify users when connecting wallets fail #2288

Merged
merged 1 commit into from Apr 7, 2023

Conversation

ukane-philemon
Copy link
Contributor

@ukane-philemon ukane-philemon commented Apr 4, 2023

Closes # 2241

UI changes:
Screenshot 2023-04-04 at 11 30 19 PM

@norwnd (#2241): As a sidenote, I'm not even sure why this connect button is there in the first place , wouldn't it be better to try re-connect wallet automatically every 30s or something?

This part is not handled in this PR. @chappjc, what do you think?

IMO, using a reconnect loop for wallet connection when are not sure that we will ever connect seems to be undesirable. Wallet connection will be attempted in any part of the client code that requires a connected wallet, plus, like the note above, If a user is missing wallet files, we are going to try forever until they take action.

@chappjc
Copy link
Member

chappjc commented Apr 4, 2023

Yeah, I don't want to replace the connect button with a background reconnect loop.

Regarding any error from connect, I'm pretty sure we're trying to show errors on the page or form near the UI element interaction that generated the error, not the notification menu (or the transient "pokes" that don't give you much time to digest the error). #1712 (comment)

@buck54321
Copy link
Member

I'd prefer to show the message on the page rather than as a tooltip.

@chappjc
Copy link
Member

chappjc commented Apr 5, 2023

How about an error message "form" that opens up with the messgae, so we don't have to pre-allocate real estate for a possible error?

@buck54321
Copy link
Member

How about an error message "form" that opens up with the messgae, so we don't have to pre-allocate real estate for a possible error?

Pre-allocating page space is the simpler option. The "form" you mention, as I understand, is an interactive (probably modal) dialog that would explain the error and possibly provide guidance or options for resolution. I do agree that such an experience is desirable. I'm not sure that we should try that in this PR.

@chappjc
Copy link
Member

chappjc commented Apr 5, 2023

Yeah, just something you can close when done reading. But didn't mean to suggest we try to provide resolutions or anything smarter that just showing the error.
It's fine to drop the error in div or something on the page. It can be hidden so it doesn't eat space unless needed. I just didn't want to sacrifice layout quality

@ukane-philemon
Copy link
Contributor Author

I just didn't want to sacrifice layout quality

Yup, these errors can be ugly and I don't quite get where in the page @buck54321 wants this kind of error message since the table where the connect btn is located already has a lot.

@buck54321
Copy link
Member

I just didn't want to sacrifice layout quality

Yup, these errors can be ugly and I don't quite get where in the page @buck54321 wants this kind of error message since the table where the connect btn is located already has a lot.

Just to echo @chappjc above

I'm pretty sure we're trying to show errors on the page or form near the UI element interaction that generated the error

I think it looks alright on the page.

buck54321/dcrdex@d165f1f...buck54321:dcrdex:ukaine-connect-err-on-page

image

@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented Apr 6, 2023

I think it looks alright on the page.

For long errors like the eth error in my PR comment, it will easily look very ugly IMO. But if you want to, we can dump everything below the button.

EDIT:
Screenshot 2023-04-06 at 2 00 22 PM

It's an awfully bad display of lengthy error messages.

I do agree that such an experience is desirable. I'm not sure that we should try that in this PR.

If using a tooltip is not a good resolution to this, we can use a general error modal that can be closed by the user when they are done reading and digesting the error, showing the whole error on the page beside the connect btn does not look desirable IMO.

@chappjc
Copy link
Member

chappjc commented Apr 6, 2023

If using a tooltip is not a good resolution to this, we can use a general error modal that can be closed by the user when they are done reading and digesting the error, showing the whole error on the page beside the connect btn does not look desirable IMO.

A tooltip is not, I think we agree. Sure, give the modal a try quick. We just don't want a major production over this.

@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented Apr 7, 2023

EDIT: UI changes.
Screenshot 2023-04-07 at 5 23 10 PM

Screenshot 2023-04-07 at 5 22 55 PM

@chappjc
Copy link
Member

chappjc commented Apr 7, 2023

Seems alright I guess. The work "Message" in "Error Message" is not needed though. It's more like "Connect Error". I also figured we'd have an "x" at the top right like all the other forms instead of an "Ok" button. BTW, it's "OK" not "Ok"

@chappjc
Copy link
Member

chappjc commented Apr 7, 2023

Anyway, you and @buck54321 work out where to put the error, but IMO it gets messy putting it right next to the button. I think the dialog form that opens is a good compromise because you can't ignore it like tooltips and notifications, and it doesn't touch the page layout.

@ukane-philemon
Copy link
Contributor Author

I also figured we'd have an "x" at the top right like all the other forms instead of an "Ok" button.

I think it's better they acknowledge that they have seen the error, the x at the top is usually for form dismissal but we can have that instead of the btn if that's better or keep both?

It's more like "Connect Error"

I wanted to be more flexible instead of tying the modal to only Connect Error.

@chappjc
Copy link
Member

chappjc commented Apr 7, 2023

I think it's better they acknowledge that they have seen the error, the x at the top is usually for form dismissal but we can have that instead of the btn if that's better or keep both?

Whatever really. The button just looks funny. It's chunky and big, and requires a translation (!).

I wanted to be more flexible instead of tying the modal to only Connect Error.

Of course, but the "Connect" part can be a variable inserted by the JS caller. If that's not trivial for whaterver reason, just "Error" is preferable to "Error Message", which looks silly.

@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented Apr 7, 2023

Updated the images above.

Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented Apr 7, 2023

Please consider buck54321/dcrdex@1e50d26...buck54321:dcrdex:error-modal-in-forms

I could use the existing form structure we have(less code) but the modal is not a "form". Still, we can run with your suggestion, I understand you prefer less code in this case.

EDIT: Done, reduced to one commit : cb81957

Signed-off-by: Philemon Ukane <ukanephilemon@gmail.com>
@buck54321
Copy link
Member

I could use the existing form structure we have(less code) but the modal is not a "form". Still, we can run with your suggestion, I understand you prefer less code in this case.

It's not about less code. The existing forms are modal, since they prevent page interaction until the form is submitted or dismissed. No reason to reinvent the wheel.

@chappjc chappjc merged commit 9e32b20 into decred:master Apr 7, 2023
5 checks passed
@chappjc
Copy link
Member

chappjc commented Apr 7, 2023

Also, this isn't the first form used this way. The "Export Wallet" form is also purely informational:

eth-export-key
(don't bother with the key, it's simnet)

@ukane-philemon
Copy link
Contributor Author

Noted.

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

3 participants