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: Display order errors on the form. #1093

Merged
merged 3 commits into from
Jul 31, 2021
Merged

Conversation

amass01
Copy link
Member

@amass01 amass01 commented May 19, 2021

Closes #1045.

As a general rule, any error encountered when submitting a form should be displayed on the form itself, not the notification bell.

We will need to skip notification in checkResponse function and just return false if there is an error, and then display res.msg in related form's error section for all other forms.

Comment on lines -1317 to -1347
// If the wallets are not open locally, they must have been opened during
// ordering. Grab updated info.
const baseWallet = app.walletMap[market.base.id]
const quoteWallet = app.walletMap[market.quote.id]
if (!baseWallet.open || !quoteWallet.open) {
this.balanceWgt.updateAsset(market.base.id)
this.balanceWgt.updateAsset(market.quote.id)
Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't relevant after #1007 which handles this for both wallets using notification.

Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

Works for me.

@chappjc
Copy link
Member

chappjc commented Jun 8, 2021

This works for me too.

But we have a more fundamental UI flaw with orders in that you can submit multiple orders before getting any response from the /api/trade endpoint request. This is most obvious with the following: (1) testnet and (2) tx split enabled. A single /api/trade can take ~10sec for BTC, and a few for DCR. It's very easy to fire off several before getting a response from the first.

Perhaps a solution to the above issue is to simply disable/gray-out the submit (Place order to buy/sell...) button until a response (success or fail) is received from the /api/trade POST. EDIT: This needs some more thought though. I wonder if there needs to be some kind of indicator of a pending order submission POST request...

@amass01
Copy link
Member Author

amass01 commented Jun 8, 2021

@chappjc Hrmm good point, I can add that.

@chappjc
Copy link
Member

chappjc commented Jun 26, 2021

To clarify my comment, I believe the following dialog should be modal and the target for errors:

image

Presently it disappears when you submit, and any amount of time can pass before server accepts the order and client gets the response and frontend is informed.

When it goes away immediately you are left hanging with no sign of an order anywhere until server processes the request and gets back to you and informs frontend. Its usually a "fast" submission so we deal with it, but it's still bad UX. And it's terrible when the delay is longer than a second as with txsplit in use on testnet or mainnet. But any server-side slowdown can do this, or connectoin drop, etc.

Hence I think the right solution is to keep this dialog opened after clicking and until a response is received. During that time, the Buy/Sell DCR button should be disabled.

Showing errors on the form where you type in rates and amounts isn't right because it's not clear what order the error is referring too. Plus it doesn't solve the fundamental UX issue I've described.

@amass01
Copy link
Member Author

amass01 commented Jun 26, 2021

This works for me too.

But we have a more fundamental UI flaw with orders in that you can submit multiple orders before getting any response from the /api/trade endpoint request. This is most obvious with the following: (1) testnet and (2) tx split enabled. A single /api/trade can take ~10sec for BTC, and a few for DCR. It's very easy to fire off several before getting a response from the first.

Perhaps a solution to the above issue is to simply disable/gray-out the submit (Place order to buy/sell...) button until a response (success or fail) is received from the /api/trade POST. EDIT: This needs some more thought though. I wonder if there needs to be some kind of indicator of a pending order submission POST request...

As discussed on matrix, @chappjc suggests we change a bit the flow, and keep the confirmation modal visible with
grayed out submit button, and in case of error display it on the confirmation modal where the user have the order's fixed numbers displayed.

@amass01
Copy link
Member Author

amass01 commented Jun 26, 2021

cc: @buck54321 thoughts regarding the above ?

@amass01
Copy link
Member Author

amass01 commented Jun 27, 2021

@chappjc updated 👍🏽

@buck54321
Copy link
Member

cc: @buck54321 thoughts regarding the above ?

@chappjc's recommendation is what I would have suggested too. Sorry for the late response.

@chappjc
Copy link
Member

chappjc commented Jul 6, 2021

Good, the dialog does not close now while the /api/trade request is processing.

But when I first entered my pass and clicked "Buy DCR" the password field cleared out, but nothing else happened until the order was accepted. Although the button is actually disabled, there really needs to be more indication that the order submission is processing:

  • Graying out the red/green button would be a bare minimum
  • Some kind of "working" indicator like a spinning / pulsing thing. We have a spinning thing for when the markets page is loading, and when max order is fetching, that could be repurposed next to or in place of the Buy/Sell button once it's clicked. See spinner and ico-spinner css classes, and the loader, marketLoader, baseSpinner, and quoteSpinner element ids.

@amass01
Copy link
Member Author

amass01 commented Jul 7, 2021

@chappjc adding a spinner in place of the button.

client/webserver/site/src/js/markets.js Outdated 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.

Spinner changes working well. Examples below.

Error with insufficient balance (spinner only flashes very briefly in this test):

order-error-quick.mp4

Error from the asset.CoinNotFoundError bug fixed in https://github.com/decred/dcrdex/pull/1128/files#r675698564, where sell DCR fails with txplit:

order-error-quick2.mp4

With a time.Sleep(2 * time.Second) in apiTrade to simulate latency, where you can see the spinner:

order-spinner.mp4

@chappjc chappjc merged commit ff59dbb into decred:master Jul 31, 2021
@amass01 amass01 deleted the orderformerror branch August 1, 2021 21:43
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.

ui: order errors should display on the form
4 participants