UI: make limit order form more responsive

[norwnd]

In this PR I'm trying to address the following:

• Price, Quantity fields should have reasonable defaults when /markets page loads (and when garbage user input gets reset), so that less clicks/actions are required by the user to place his limit order
• validation for Price, Quantity fields should be more responsive, so that the user has a better sence of why his inputs don't make sense
• Place order to buy ... button press should fail right away when Total exceeds Max Buy, instead of allowing the user to proceed only to fail later on

Here is a demo to get the general idea - https://skynetfree.net/AADCen1qLW87koj0oLmJpgLg9cOMInagd1W0CS8dOQdtkQ (couldn't make reasonably small recording to fit 10MB Github limit with Kap to embed it here)

Closes #1908.

[chappjc]

When pushing updates to reviewable PRs, please try to ensure there is always a way to see just what was changed since the last push. Either add commits, which we will squash before merge, or force push once without changing the merge-base so that when we view the "force-pushed" diff it shows only what was changed in your PR. If the merge-base changes, as with the latest force-push, it is mixed up with whatever was changed on master, making it more difficult to review. In this instance, is not too hard to zero in on just what you changed, but it's not something reviewers should have to do.

[norwnd]

it's not something reviewers should have to do.

Sorry, makes sense, something I'll try to pay attention to in the future.

[chappjc]

Sorry, makes sense, something I'll try to pay attention to in the future.

Not a problem. It wasn't documented, until just now. :) https://github.com/decred/dcrdex/wiki/Contribution-Guide#pull-requests-prs

[JoeGruffins]

Looks really cool! Noticing a couple things while testing:

Can you also prevent negative values?

Is the max order calculation correct? I don't have this much btc to sell.

[buck54321]

Please tweak the "cache busters" in the url queries here and here. They just need to be changed in some way to force reloading of web assets.

[buck54321]

lostSizeBox -> lotSizeBox

[buck54321]

You have resolved this, but have not changed anything.

[norwnd]

Hmm, I guess got distracted by something else and then forgot, thanks for catching it again! Should be fixed now.

[buck54321]

If using transition, why not specify it in CSS? The prevTransition stuff is likely unnecessary. Did you have a conflict with another transition value?

[buck54321]

Another thing to consider is that the transition attribute applies both direction, which means we can only fade in -> fade out, we can't say flash on -> fade out, which I think would be a better indicator.

A couple of suggestions:

1. Use Doc.animate to do a flash -> fade
2. For inputs, at least, animate the element's outline attribute rather than the background color.

[norwnd]

why not specify it in CSS?

My knowledge of CSS and some front-end stuff is a bit rusty, I've rewrote this piece with Doc.animate; please let me know if you had a different solution in mind.

The prevTransition stuff is likely unnecessary. Did you have a conflict with another transition value?

Not really, the example I was following was doing that - my guess would be it's just "general purpose code", sort of best practice; removed.

I've applied those suggestions, seem - looks better indeed!

[buck54321]

I do not agree that we should have a default here. It is not standard on any exchange's order form that I've used.

[norwnd]

Some reasons for why having defaults there might be a good idea:

• it provides an example, emphasizing the Lot concept (present in Dex, and not on most other exchanges, at least not so explicitely) = more streamlined experience for users trying out Dex for the very first time
• many first-time users will probably want to start with 1 lot, so less clicks overall = good for onboarding (and it doesn't affect those who want to increase this to 2, 3, ... except, as is implemented right now - one has to erase those defaults first to put in another value, this is probably a deal-breaker, but I hope this can be solved somehow)
• having those defaults there allows for Total calculated on page load, showing the user all the order-form parts in complete (and also happens to show min trade size which is neat)
• some manual UI testing is simpler with defaults, since I can place a bunch of limit order faster

If that's convincing enough, then I can try to address that "deal-breaker" thing, if not - I'll just remove defaults.

[buck54321]

Other than illustrating the lot concept, all of these points could be made about any exchange interface, yet nobody does this. I don't think it's a good idea to pre-fill critical fields on the order form.

[chappjc]

Agree. Plus there are events that can force reload the page, and if a valid order size replaces what you had entered before, you may not notice, and end up submitting the wrong order.

[norwnd]

Interesting, ok - will remove.

How do you feel about slider, perhaps in the next PR ? Seems like a popular feature these days.

[buck54321]

Love the slider!
I also use these buttons sometimes

[norwnd]

Haven't seen those buttons (mid, bid, ...) before, but I'll give a try to slider later on in a separate PR.

[buck54321]

I'm not feeling this color.

[norwnd]

I'm not feeling this color.

Changed to red, or do you have something else in mind ?

[buck54321]

I'd start by seeing if any of the constants in application.scss would work. Two that jump out at me are

$sellcolor_dark: #e95e5e;
$sellcolor_light: #99302b;

And then set the outline color in both market.scss.

div[data-handler=markets] {
  input {
    outline: 2px solid $sellcolor_light;
  }
}

and market_dark.scss.

body.dark {
  div[data-handler=markets] {
    input {
      outline: 2px solid $sellcolor_dark;
    }
  }
}

You can also choose new colors for this purpose, but different colors will look better with light and dark mode, and you should spend some playing with the color picker in the developer console to find a color that blends well with others.

[buck54321]

Actually, I don't know that would work with the animation. I suppose you would need to define the rgb values in markets.ts after all. You can pick which color (dark/light) using State.isDark().

See e.g. solution offered in #1921 (comment)

[norwnd]

Yeah, I don't think I can make css can work with animation, or at least don't know how to make it workable and simple,

I went the route you've suggested below.

Still have a couple of bugs to resolve, but the colors you've suggested below seem OK to me (I only adjusted transparency a bit).

[norwnd]

Is the max order calculation correct? I don't have this much btc to sell.

• By max you probably meant total, Max buy being 0 seems fine to me

• The calculation of Total value (for large values) seems off (Total value on your screenshot is not what 1e+50 / 10 should be); It seems to me the reason is parseFloat is used to parse all those order-form inputs which returns number that's represented as float / double (32 / 64 bit). There seems to be no native decimal of arb precision in JS based on brief googling, there are some libs out there, for example: https://github.com/MikeMcl/decimal.js/ or https://github.com/MikeMcl/big.js/, however I don't know what your opinion of using a lib for this is, this particular issue probably isn't that nasty to warrant such usage, maybe there are other places where UI might benefit from using precise decimals though.

• The fact that "you don't have this much BTC to sell" is actually on purpose, allowing the user to fill in whatever (positive) numbers he likes so that he can estimate Total without having the actual funds in his wallet; the actual balance-check will happen when Place order ... button is pressed.

Can you also prevent negative values?

Should be fixed now, just note that validation kicks in only after focus leaves input field (which seems to me less intrusive, based on what testing I've done, than validating-and-resetting-to-default-value on each key-press which gets jerky).

@JoeGruffins, guys ^ if you have any suggestions on this

[norwnd]

I've addressed reviews above, and in the process had to rewrite change & keyup event handling on limit-order form (also touching other parts of markets page a bit), hopefully you don't mind that.

Also cleaned up Market-Order form tiny bit (e.g. Rate step and Total have no business being present there). Hope to improve it slightly in a separate PR later.

Couldn't quite figure out yet why linter fails though, on line 571 I don't even have 7th column ... (edit: fixed)

Error: src/js/markets.ts(571,7): error TS2532: Object is possibly 'undefined'.
Error: Process completed with exit code 2.

[norwnd]

Rebased & force-pushed,

When pushing updates to reviewable PRs, please try to ensure there is always a way to see just what was changed since the last push. Either add commits, which we will squash before merge

I think I should have asked about force-push first, still learning how Github treats diffs, didn't think it will update all the commits before the last one I added (since none of them changed except for resolving conflict in bodybuilder.tmpl, unlike when I squashed a bunch of commits above - where you originally pointed this out) after rebase.

[buck54321]

It probably seems like I’m on a power trip, but I assure you that’s not the case. There are some nice improvements here that I really want to get in. But we’re not going to use them as a Trojan horse to get a bunch of unnecessary changes in just because you want it styled your way. That's a recipe for disaster. I won’t merge a 600 LOC PR without additional approvals, and I can’t speak for others, but my guess is that this isn’t getting reviewed because nobody can pin down what all of the changes are fixing. Most of them aren’t fixing anything.

I'm happy to spin off a minimalist PR that has all the fixes, if that helps. I swear that my only goal is to get your fixes in.

[chappjc]

Now that we've been through several iterations and it's functionally what we want (afaict), I'm happy to approve after we squash away the fixups and stuff to a few commits with model commit messages. The way I'd like to merge is with multiple independent commits (assuming there are independent bits here).

I agree with @buck54321 that it has been hard to see through all the extraneous changes, and that's why review has been sluggish on this. I think it's more minimal now, so let's massage the branch to the point that it's ready to click "Rebase and merge" and we'll end up with clear and well commented commits on master.

[norwnd]

The only thing that's not highly-tied/related to Limit-form changes in this PR is Market-order form being split-off into it's own view (HTML + separate JS handling), the rest of the changes are pretty much adjustments needed to make Limit-form work smoothly.

The risk of making a mistake (introducing bugs) splitting off this Market-order stuff into separate commit is pretty high compared to the benefit of simpler-diff, I think,
and to do that I'd need to make old Market-form work with new Limit-form (which is a challenge in itself, if even possible to do in a reasonably clean way) because we probably don't want non-working commit in master; and we'll be just throwing away this effort in a follow-up Market-order stuff commit, assuming you are on board with this split-off, better described here:

having all 4 order types: Limit/Market+Buy/Sell managed by same view seems have resulted into tight-coupling of some code in markets.ts. To keep code-complexity manageable, I've split off Market-Sell order stuff into its own separate view (Limit Buy/Sell forms are almost identical, can't say same for Market orders). Market-Buy form was already handled by separate view (see MARKET BUY ORDER in markets.tmpl)

So, while I do appreciate the security concerns you guys have raised, I think the most practical way to go about it is to take our time and carefully review what's implemented here in one piece. Since (from what I see in master) you don't squash PR-commits before merging, I've pushed a new PR: #2043 to combine these 23 commits into 1, summarising in commit message all the things it changes (this PR can be closed in favor of #2043, I just didn't want to force-rewrite commit history in this one, in case you'd want to preserve it for some reason).

[norwnd]

Superseded by #2043.