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: Order submit button state management #2534

Merged
merged 7 commits into from
Nov 5, 2023

Conversation

peterzen
Copy link
Member

@peterzen peterzen commented Sep 21, 2023

This PR modifies the behavior of the order submit button so that it prevents the user from submitting the order if it would fail due to the below conditions:

  • quantity is 0
  • buy or sell quantity is more than available balance

Closes #2527

@peterzen peterzen marked this pull request as ready for review September 21, 2023 23:57
@JoeGruffins
Copy link
Member

Will also say insufficient balance if lots are set to zero but have balance.
image

@peterzen peterzen force-pushed the order-submit-btn-state branch 2 times, most recently from 6769d5b to 0e97533 Compare September 26, 2023 12:47
@peterzen
Copy link
Member Author

Will also say insufficient balance if lots are set to zero but have balance. image

Thanks for catching that, it's fixed.

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.

For other reasons why a user would not be able to trade, such as the bond pending or one of the wallets not yet being created, we are removing the order form completely. See resolveOrderFormVisibility in markets.ts. Instead of the order form, there is a large message displayed saying why the user cannot trade.

Personally, I don't think the order form should be shown if the user has no balance in either of the assets. Why should they be entering a rate and # of lots if they cannot trade anyways? Also, if only one of the assets has a balance, then you can just grey out the Buy/Sell button on top that switches to the other side of the market.

Thoughts? @buck54321 @JoeGruffins ?

@martonp
Copy link
Contributor

martonp commented Oct 1, 2023

For markets involving ETH/Tokens, if there is no ETH balance in the wallet, then the user will not be able to trade at all, even if they are buying ETH or the ETH token. The user needs ETH to redeem.

@peterzen
Copy link
Member Author

peterzen commented Oct 1, 2023

Personally, I don't think the order form should be shown if the user has no balance in either of the assets. Why should they be entering a rate and # of lots if they cannot trade anyways? Also, if only one of the assets has a balance, then you can just grey out the Buy/Sell button on top that switches to the other side of the market.

A new user will most likely want to click around to discover the UI/UX before depositing funds - if they can't find the order form anywhere that's unusual/confusing and will likely to generate support questions.

What problem would the hidden form solve?

@peterzen peterzen force-pushed the order-submit-btn-state branch from 0e97533 to 6bbd0c6 Compare October 1, 2023 08:44
@martonp
Copy link
Contributor

martonp commented Oct 2, 2023

Never mind, you're right, it's better like this. I do notice a few issues though:

  1. When I hover over the button, I see a question mark then the tooltip only shows up a few seconds later. Maybe the error message should be the text of the button?
  2. When I switch markets, the button is not updated.
  3. When I deposit funds, and the insufficient balance error is no longer true, the button should update.
Screen.Recording.2023-10-01.at.10.22.45.PM.mov

@peterzen peterzen force-pushed the order-submit-btn-state branch from 8a388a7 to c9fbf92 Compare October 3, 2023 00:18
@peterzen
Copy link
Member Author

peterzen commented Oct 3, 2023

Never mind, you're right, it's better like this. I do notice a few issues though:

1. When I hover over the button, I see a question mark then the tooltip only shows up a few seconds later. Maybe the error message should be the text of the button?

Fixed the cursor to not-allowed, the tooltip behaves correcly now.

2. When I switch markets, the button is not updated.

Solved - when switching markets the button changes to a disabled state as the form values are cleared.

3. When I deposit funds, and the insufficient balance error is no longer true, the button should update.

The button state updates correctly now, also refactored the update logic to make the changes more readable.

@martonp
Copy link
Contributor

martonp commented Oct 6, 2023

Screen.Recording.2023-10-06.at.12.20.48.AM.mov
Screen.Recording.2023-10-06.at.12.47.51.AM.mov

For me the tooltip still looks the same. Also, I noticed that the market order section is broken. For market buys, the button is always disabled, even if the balance is enough. The market sells button also has some strange behavior you can see in the videos.

@peterzen peterzen force-pushed the order-submit-btn-state branch from 8c5e795 to 9d147bb Compare October 6, 2023 13:52
@peterzen peterzen changed the title ui: Disable order submit button if insufficient balance is available ui: Order submit button state management Oct 6, 2023
@peterzen
Copy link
Member Author

peterzen commented Oct 6, 2023

For me the tooltip still looks the same. Also, I noticed that the market order section is broken. For market buys, the button is always disabled, even if the balance is enough. The market sells button also has some strange behavior you can see in the videos.

Thanks for testing this, the bug causing those issues is fixed. I've also added a test for the rate field being non-zero, and sorted the problem of the UI not always being updated when a new balanceNote came in.

@martonp
Copy link
Contributor

martonp commented Oct 11, 2023

The logic looks good now. How do you feel about this clean up though? 0487795

Also, the hover over the button still has the same behavior for me. I see that error icon for a bit before the tooltip shows up. How do you feel about just updating the button text?

@peterzen
Copy link
Member Author

The logic looks good now. How do you feel about this clean up though? 0487795

Thanks for that, added.

Also, the hover over the button still has the same behavior for me. I see that error icon for a bit before the tooltip shows up. How do you feel about just updating the button text?

Would it be clearer if the cursor stayed the default, instead of changing to the error icon on hover?

The button might break the layout if we changed the text, translated strings might be longer than the English default and even that is at the limits.

image

@peterzen peterzen force-pushed the order-submit-btn-state branch from 54dbfe0 to 827d9db Compare October 11, 2023 20:58
@martonp
Copy link
Contributor

martonp commented Oct 15, 2023

Would it be clearer if the cursor stayed the default, instead of changing to the error icon on hover?

Yeah I think the error icon is kind of confusing. I've never really seen that before.
I guess no need for the button text updating. It's clear enough.

@martonp
Copy link
Contributor

martonp commented Oct 15, 2023

I think a better UI would be to actually just allow the button, but when you click it, it does a form validation with error message(s) below the invalid fields.

@peterzen peterzen force-pushed the order-submit-btn-state branch 2 times, most recently from 2a3e599 to 4b19f04 Compare October 16, 2023 22:53
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.

Is this right?

button-viz.mp4

client/webserver/site/src/js/markets.ts Show resolved Hide resolved
client/webserver/site/src/js/markets.ts Outdated Show resolved Hide resolved
client/webserver/site/src/js/markets.ts Outdated Show resolved Hide resolved
Comment on lines -2515 to -2516
// If we're not showing the max order panel yet, don't do anything.
if (!mkt.maxSell) break
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Why?

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 way the max order panel and the order button update when the balance does.

client/webserver/site/src/js/markets.ts Show resolved Hide resolved
@peterzen
Copy link
Member Author

Is this right?
button-viz.mp4

Could you please repost the vid, it won't render:
image

@buck54321
Copy link
Member

Hmm. Well basically, for a limit buy order, the button state was not reactive to changes in the lots field.

@peterzen
Copy link
Member Author

Hmm. Well basically, for a limit buy order, the button state was not reactive to changes in the lots field.

Was there anything logged in the console? Was the button tooltip referring to no balance or empty order quantity?
I can't reproduce the problem.

client/webserver/site/src/js/markets.ts Outdated Show resolved Hide resolved
client/webserver/site/src/js/markets.ts Show resolved Hide resolved
client/webserver/site/src/js/markets.ts Show resolved Hide resolved
client/webserver/site/src/js/markets.ts Outdated Show resolved Hide resolved
client/webserver/site/src/js/markets.ts Show resolved Hide resolved
client/webserver/site/src/js/markets.ts Show resolved Hide resolved
@peterzen peterzen force-pushed the order-submit-btn-state branch from 94bcbd4 to 80d55be Compare October 29, 2023 16:15
@peterzen
Copy link
Member Author

peterzen commented Oct 29, 2023

This commit is somewhat unrelated but feels like it belongs here

@peterzen peterzen force-pushed the order-submit-btn-state branch from f8a21f5 to 204a7d0 Compare October 29, 2023 22:38
@peterzen peterzen force-pushed the order-submit-btn-state branch from afea516 to eacaacf Compare November 2, 2023 23:52
@buck54321 buck54321 merged commit 08c04a1 into decred:master Nov 5, 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.

ui: Block buy/sell button when balance is 0
4 participants