ui: Bin order tables by rate. #1090

martonp · 2021-05-14T18:07:08Z

This PR updates the order tables on the market UI. Instead of displaying each individual order's quantity and rate on a separate row, the total quantity available at each rate will be displayed. Also, as discussed in matrix, epoch orders will be binned along with all other orders, and the check mark will no longer be displayed.

Closes #945.

JoeGruffins

Looks perfect to me.

chappjc · 2021-06-08T21:54:51Z

Appears to work as indented.

However, we should be aware that while this is a UX improvement to some, it actually hides some important information that I hope we can strive to reveal again, perhaps in a future PR.

Take the following simnet buy side entirely comprising my own orders:

Another party just sees the Buy Orders table and has no idea if the 20 DCR best bid is one order or two. This may be an important factor in the user deciding if the transaction will be maximally efficient if they decide to sell. Even if the stars align and those maker orders do get batched by the taker/seller's redeem txn, that redeem is still larger than it would have been if there were just one match instead of two. Plus they still need to make two contracts for the two countparties.

Then consider the 30 DCR at 0.003945. If a user's sell fills 20 DCR at that price level, what would they match against? They would need to see the unbinned orders sorted by time stamp (as before) to know what to expect (assuming no other takers got to those bids first).

martonp · 2021-06-09T00:13:02Z

@chappjc There could be a little pop up that shows up when you hover over each row that contains this information. I may be wrong, but I don’t think people would be so concerned with this if the lot size is set properly and the transaction fees are negligible compared to the order. It definitely can’t hurt to have it there though.

buck54321

Looking good for a first pass. I do want to separate epoch orders from booked orders.

As far as the UI question, I think a good first approach would just be to label the quantity of orders in the bin. e.g.

You could do that in a separate PR too.

buck54321 · 2021-06-16T13:06:26Z

client/webserver/site/src/js/markets.js

+    if (row && row.getRate() === 0) row = row.nextSibling
    while (row) {
-      if ((order.rate < row.order.rate) === order.sell) {
+      if (order.rate === row.getRate()) {
+        row.insertOrder(order)
+        return
+      } else if ((order.rate < row.getRate()) === order.sell) {
+        const tr = this.orderTableRow([order], cssClass)
        tbody.insertBefore(tr, row)
        return
      }
      row = row.nextSibling
    }


You'll need to figure out a way to separate epoch queue orders and booked orders and create a row for each.

buck54321 · 2021-06-16T13:08:29Z

client/webserver/site/src/js/markets.js

+      if (!tr.orderBin.length) {
+        tr.remove()
+      } else {
+        tr.updateQty()
+      }


Can be two lines

buck54321 · 2021-06-16T13:08:37Z

client/webserver/site/src/js/markets.js

+      if (!tr.orderBin.length) {
+        tr.remove()
+      } else {
+        tr.updateQty()
+      }


Can be two lines.

martonp · 2021-06-20T17:36:16Z

@buck54321 I've added a new column that shows the number of orders, let me know what you think. Also the epoch and non-epoch orders are now in separate rows.

client/webserver/site/package.json

This reverts commit ac7d1f0.

chappjc

@buck54321 I've added a new column that shows the number of orders, let me know what you think. Also the epoch and non-epoch orders are now in separate rows.

I think it uses a bit too much space. Just a number and no bulky column header like @buck54321 showed would be fine IMO. Here's what I see now:

But it gets a little glitchy at various window widths, putting "# Orders" on two rows, and often creating a horizontal scroll bar:

Otherwise, it works as intended. There's a separate row for epoch orders, and when booked, it merges that with the booked orders row and updates the # Orders count. No issues in browser JS console. Overall nice work, but I don't think the column header adds much, possibly even the separate column is unnecessary if the number is just after the quantity in the same column with appropriate styling. Like buck suggested, but perhaps with a tweaked styling:

martonp · 2021-07-06T20:58:19Z

@chappjc I've updated it. This definitely looks better. I thought it would be confusing to just have a number there without a column header, but I added a tooltip which explains what the number means.

chappjc

I like it. Nice work!

@buck54321 LGTM, some frontend changes since your last review though.

buck54321

Good stuff. I've made a few more suggestions to round things out.

buck54321 · 2021-07-07T12:04:07Z

client/webserver/site/src/js/markets.js

+      qtyDiv.innerText = qty.toFixed(8)
+      ordersDiv.innerText = tr.orderBin.length


Let's only show the qtyDiv when there is more than one order.

buck54321 · 2021-07-07T12:05:47Z

client/webserver/site/src/css/market.scss

@@ -427,6 +427,12 @@ table.balance-table button:hover {
  }
 }

+div.numorders {
+  color: white;


No white text, please. Inherited color will work fine once you get the background colors adjusted.

buck54321 · 2021-07-07T12:13:01Z

client/webserver/site/src/css/market_dark.scss

@@ -24,6 +24,11 @@ body.dark {
    }
  }

+  div.numorders {
+    background-color: rgb(25, 25, 180);


Let's darken this up quite a bit. Maybe like a #141488.

Maybe we could test out one of our primary colors: https://decred.org/brand/

Keep the hue / rgb ratio and tweak the brightness? If that clashes with the rest of the DEX scheme, no biggie.

The first one is a lighter version of the dark blue primary color and the second is @buck54321 's suggestion. Very similar, but I think @buck54321 's suggestion is a closer match to the dex logo, which doesn't seem to be using any of the Decred colors.

k, no sweat. Just wanted to try to keep us on-brand if possible, but best to stay internally consistent with other dex styles unless we rework the whole scheme.

But honestly, I like the top one you showed best. If you guys think it looks funny, we should go with the matching one.

buck54321 · 2021-07-07T12:15:35Z

client/webserver/site/src/css/market.scss

+  border-radius: 3px;
+}


The coloring is a little bright. It should blend in and match existing styles more closely.

To do that, I added line-height: 1; padding: 1px; here, and then removed the px-1 class and added the align-items-center class to the wrapper div. You might want to play with font-size: 0.9em too, but the font-size is already pretty small here so I'm not sure we want to push it.

buck54321 · 2021-07-07T12:33:25Z

client/webserver/site/src/css/market.scss

@@ -427,6 +427,12 @@ table.balance-table button:hover {
  }
 }

+div.numorders {
+  color: white;
+  background-color: rgb(94, 94, 130);


Let's lighten this up quite a bit. Maybe like a #c5c5e6

buck54321 · 2021-07-07T12:55:03Z

client/webserver/site/src/js/markets.js

+    tr.insertOrder = (order) => {
+      tr.orderBin.push(order)
+      updateQtyTD()
+    }
+    tr.updateOrderQty = (token, qty) => {
+      for (let i = 0; i < tr.orderBin.length; i++) {
+        if (tr.orderBin[i].token === token) {
+          tr.orderBin[i].qty = qty
+          updateQtyTD()
+          return true
+        }
+      }
+      return false
+    }
+    tr.removeOrder = (token) => {
+      const index = tr.orderBin.findIndex(order => order.token === token)
+      if (index < 0) return false
+      tr.orderBin.splice(index, 1)
+      if (!tr.orderBin.length) tr.remove()
+      else updateQtyTD()
+      return true
+    }
+    tr.removeEpochOrders = (newEpoch) => {
+      tr.orderBin = tr.orderBin.filter((order) => {
+        return !(order.epoch && order.epoch !== newEpoch)
+      })
+      if (!tr.orderBin.length) tr.remove()
+      else updateQtyTD()
+    }
+    tr.getRate = () => {
+      return tr.orderBin[0].rate
+    }
+    tr.getEpoch = () => {
+      return tr.orderBin[0].epoch
+    }
+    tr.compare = (order) => {


This is good stuff, and it makes the semantics of updating these rows really clean, but once we're adding seven new functions as custom properties to an td element, that's a big clue to me that we should just write a custom class to manage the element and make these class methods.

I've added a manager class that has a reference to the td element the td element has a reference to the manager class. It's definitely a lot cleaner.

buck54321 · 2021-07-07T12:58:03Z

client/webserver/site/src/js/markets.js

+      const mainDiv = qtyTD.children[0]
+      const [qtyDiv, ordersDiv] = mainDiv.children


Try using the data-tmpl/Doc.tmplElement system used throughout, and assigning these elements as properties of the custom class mentioned in my other comment.

chappjc

Re-approving latest changes.

buck54321

If I place two identical orders in quick succession, the epoch orders are not binned.

Then, when the epoch expires, and the orders are booked, they are properly grouped, but the epoch check mark is still displayed.

buck54321 · 2021-07-09T18:19:38Z

client/webserver/site/src/js/markets.js

+    return this.sell
+  }
+
+  compare (order) {


I try to doc all methods, but this method in particular could use a docstring.

buck54321 · 2021-07-09T18:29:51Z

client/webserver/site/src/js/markets.js

+    bins.push(currEpochBin)
+    return bins.filter(bin => bin.length > 0)
+  }
+
  /* loadTables loads the order book side into its table. */
  loadTableSide (sell) {
    const bookSide = sell ? this.book.sells : this.book.buys
    const tbody = sell ? this.page.sellRows : this.page.buyRows
    const cssClass = sell ? 'sellcolor' : 'buycolor'


cssClass is unused outside of OrderTableRowManager (via orderTableRow). I'd recommend moving this cssClass stuff into the OrderTableRowManager constructor, and just passing the sell boolean as an argument instead.

martonp · 2021-07-10T18:15:29Z

@buck54321 The bug is fixed now.

Bin order tables by rate.

390bcaf

JoeGruffins approved these changes May 18, 2021

View reviewed changes

buck54321 requested changes Jun 16, 2021

View reviewed changes

Marci added 3 commits June 19, 2021 22:55

Merge branch 'master' into binOrderTable

3f3227b

Update.

88d8bb7

Update.

ac7d1f0

martonp commented Jun 20, 2021

View reviewed changes

client/webserver/site/package.json Outdated Show resolved Hide resolved

Revert "Update."

0877bca

This reverts commit ac7d1f0.

chappjc self-requested a review July 5, 2021 16:09

chappjc requested changes Jul 6, 2021

View reviewed changes

Update.

f617f0d

chappjc approved these changes Jul 6, 2021

View reviewed changes

buck54321 requested changes Jul 7, 2021

View reviewed changes

Marci added 2 commits July 8, 2021 15:27

Update.

8318a0d

ordersbin -> orderbin.

051ee79

chappjc approved these changes Jul 9, 2021

View reviewed changes

buck54321 requested changes Jul 9, 2021

View reviewed changes

Update.

aa10aa2

buck54321 approved these changes Jul 20, 2021

View reviewed changes

chappjc merged commit f7086ab into decred:master Jul 20, 2021

martonp deleted the binOrderTable branch December 20, 2022 22:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui: Bin order tables by rate. #1090

ui: Bin order tables by rate. #1090

martonp commented May 14, 2021

JoeGruffins left a comment

chappjc commented Jun 8, 2021 •

edited

Loading

martonp commented Jun 9, 2021

buck54321 left a comment •

edited

Loading

buck54321 Jun 16, 2021

buck54321 Jun 16, 2021

buck54321 Jun 16, 2021

martonp commented Jun 20, 2021

chappjc left a comment

martonp commented Jul 6, 2021

chappjc left a comment

buck54321 left a comment

buck54321 Jul 7, 2021

buck54321 Jul 7, 2021

buck54321 Jul 7, 2021

chappjc Jul 7, 2021

martonp Jul 8, 2021

chappjc Jul 8, 2021 •

edited

Loading

buck54321 Jul 7, 2021

buck54321 Jul 7, 2021

buck54321 Jul 7, 2021

martonp Jul 8, 2021

buck54321 Jul 7, 2021

chappjc left a comment

buck54321 left a comment

buck54321 Jul 9, 2021

buck54321 Jul 9, 2021

martonp Jul 10, 2021

martonp commented Jul 10, 2021

		qtyDiv.innerText = qty.toFixed(8)
		ordersDiv.innerText = tr.orderBin.length

		const mainDiv = qtyTD.children[0]
		const [qtyDiv, ordersDiv] = mainDiv.children

ui: Bin order tables by rate. #1090

ui: Bin order tables by rate. #1090

Conversation

martonp commented May 14, 2021

JoeGruffins left a comment

Choose a reason for hiding this comment

chappjc commented Jun 8, 2021 • edited Loading

martonp commented Jun 9, 2021

buck54321 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martonp commented Jun 20, 2021

chappjc left a comment

Choose a reason for hiding this comment

martonp commented Jul 6, 2021

chappjc left a comment

Choose a reason for hiding this comment

buck54321 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc left a comment

Choose a reason for hiding this comment

buck54321 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martonp commented Jul 10, 2021

chappjc commented Jun 8, 2021 •

edited

Loading

buck54321 left a comment •

edited

Loading

chappjc Jul 8, 2021 •

edited

Loading