ui: restore vscroll to buy/sell orders tables

[martonp]

Closes #1091

[chappjc]

I'm seeing some side-effects:

The ordertables and the "Your Orders" have both light gray horizontal and vertical bars that weren't there before.

I can't seem to Inspect them either. The above is Firefox. It's even crazier in Chromium:

BTW, there are other pre-existing scroll quirks if you start playing around. One is the main page's horizontal scroll that is there on load but will disappear if you change the window height. Another is that orderform's or one of it's parents like orderPanel like to bee too tall to the point of pushing the Your Orders table off the screen until you refresh and then it is shorter.

[martonp]

@chappjc Wow.. I don't see any of those issues when I try on either firefox or brave. Can you try to clear your cache or run in a private tab? Also, are you using windows? Maybe it displays differently on the windows versions of the browsers.

[chappjc]

I hard-refreshed both, but will try again. Linux, Firefox developer edition (91b9) and Chromium 92. Will try with
Epiphany too.

BTW, one thing we like to do with frontend change is to bump the url queries on style.css and entry.js in bodybuilder.tmpl. In a perfect world we'd only do that on release, but it tends to solve cache issues during dev.

[chappjc]

I don't see the problem on Epiphany (essentially Safari), but it's definitely there on FF and Chromium. It's not easy to see on FF, except in the Your Orders table.

[chappjc]

Looks like we should use auto for overflow-y
https://developer.mozilla.org/en-US/docs/Web/CSS/overflow

[chappjc]

Actually, I noticed that the style of the scrollbar is quite inconsistent between browsers too, so think we should use our stylish-overflow class.

[chappjc]

Although I had suggested overflow: auto, I think we should just not set overflow here at all since the parent elements already are stylish-overflow class, which makes a scroll bar with consistent appearance across browsers.

[martonp]

@chappjc I've enabled scrollbars on mac and everything looks pretty good now. Let me know what you see.

[chappjc]

I'm pretty uncertain about the use of JS to compute heights here. It's also appearing to set heights sub-optimally. Perhaps other devs can make a suggestion regarding responsive design and flex to achieve this more simply with just bootstrap classes.

[martonp]

@chappjc I found a much nicer way to do this without JS. I'll still review this a bit later, but I wanted to check it in so no one else wastes time on it.

[martonp]

This is ready for another look.

[buck54321]

The thing is, we already have stylish-overflow applied to those divs, and it used to work.

dcrdex/client/webserver/site/src/html/markets.tmpl

Line 78 in 1640b70

<div class="max-h-100 stylish-overflow">

We need to figure out why it stopped working. It was almost certainly a change from #1016.

[chappjc]

That's what I realized as well. #1142 (comment)
I think the responsive design changes lacked a max-height, but not sure.

[martonp]

What I currently have addresses the issues that were created in the responsive design. The two order tables and the order panel are all in a flex row (orders-left), and the height of the row depends on the largest column in the row. As more rows are added to buy/sell order tables, the entire row expands. Instead, what we want is the buy/sell order tables to depend on the height of the order panel. This is the same relationship the userorders div has to orders-left.

The userorders div has the fill-abs class and a minimum height, so on large screens its height depends on the height of orders-left and on smaller screens it has the minimum height. What I have now for the buy/sell tables is the same, except on large screens it has the same properties as fill-abs and on smaller screens it just has a max-height. The benefit of this is that it avoids additional space when there are very few rows (this shouldn't matter in practice).

I will update userorders to use a max height instead of a min height as well.

[JoeGruffins]

Tried all the sizes and configurations I could think of on firefox and looks fine.

[chappjc]

What I currently have addresses the issues that were created in the responsive design. The two order tables and the order panel are all in a flex row (orders-left), and the height of the row depends on the largest column in the row. As more rows are added to buy/sell order tables, the entire row expands. Instead, what we want is the buy/sell order tables to depend on the height of the order panel. This is the same relationship the userorders div has to orders-left.

The userorders div has the fill-abs class and a minimum height, so on large screens its height depends on the height of orders-left and on smaller screens it has the minimum height. What I have now for the buy/sell tables is the same, except on large screens it has the same properties as fill-abs and on smaller screens it just has a max-height. The benefit of this is that it avoids additional space when there are very few rows (this shouldn't matter in practice).

^^ Thanks for this. Going forward, this kind of description is desirable for the git commit messages, for the purposes of assisting review by describing the root cause of the issue and the approach used to fix it, and to express the intent of the code changes for posterity.

[martonp]

@chappjc What do you mean? Should I copy this to the description of the PR?

[chappjc]

I'm referring to the git commit messages. So instead of a one sentence like "Add vscroll to buy/sell order tables." or "Update.", you'd describe the change like you did well in that github comment. No need to update anything now though, just going forward.

When I merge the PR I will squash down and describe the change in terms of what was done to restore the vscroll, why it was broken, and any other improvements. So ideally the git commit messages would already have this done.

[chappjc]

I feel like there should be a way to use the breakpoints defined in application.scss, no? Like @media-breakpoint-down(md) and @media-breakpoint-up(lg) mixins instead of these @media(min/max...) lines

dcrdex/client/webserver/site/src/css/application.scss

Lines 17 to 21 in 90b1d92

	$grid-breakpoints: (
	xs: 0,
	sm: 576px,
	md: 768px,
	lg: 992px,

If doing so makes things more complex, then this is fine, but it seems a bit fragile because we risk changing one but not the other.

[martonp]

With @media-breakpoint-down and @media-breakpoint-up that are different from what we are using, but what I have now has the same effect.

[chappjc]

Bummer the mixin wasn't quite right. I'm still uncertain why it wouldn't fit the bill here though. Basically the "docs" at https://mdbootstrap.com/docs/standard/layout/breakpoints/ and the code in node_modules/bootstrap/scss/mixins/_breakpoints.scss indicate it's doing basically what you're doing except with a - 0.02 instead of - 1 for the media-breakpoint-down impl, which does still sound appropriate.

[martonp]

The $grid-breakpoints variable that we are using in application.scss is different from the one in bootstrap. The xxl size we have is 1750px instead of 1400px. The lg size is the same but I think it's better to be consistent.

[chappjc]

Ok, I see your concern now. But the map we define in application.scss seems to override the one in bootstrap's _variables.scss. This seems to be on account of the use of the SASS !default flag (https://sass-lang.com/documentation/variables#default-values)

https://github.com/twbs/bootstrap/blob/6ffb0b48e455430f8a5359ed689ad64c1143fac2/scss/_variables.scss#L195-L201

With the mixin:

[chappjc]

Er, bootstrap link: https://getbootstrap.com/docs/4.6/getting-started/theming/#variable-defaults

[martonp]

The issue was that I was using the same size in both the up and down case, not that it was using the incorrect variable. It's fixed now.

[chappjc]

Just making a note that when we update to Bootstrap 5, there's a breaking change we might not catch:

Media query mixins parameters have changed for a more logical approach.

• media-breakpoint-down() uses the breakpoint itself instead of the next breakpoint (e.g., media-breakpoint-down(lg) instead of media-breakpoint-down(md) targets viewports smaller than lg).
• Similarly, the second parameter in media-breakpoint-between() also uses the breakpoint itself instead of the next breakpoint (e.g., media-between(sm, lg) instead of media-breakpoint-between(sm, md) targets viewports between sm and lg).

https://getbootstrap.com/docs/5.0/migration/#utilities