[Table] maximize() doesn't work as intended (was: Pinned header and scrollable body)

Chris-Xie commented 6 years ago

Choose the section that applies to you and use the template to help us figure out how we can help you.

Bug report

We would like to have the capability to pin the table header while scrolling the table with many rows so that only the body part are scrolled.

Tradeshift UI version affected

latest

Expected Behavior

Actual Behavior

Steps to reproduce

Screenshots (optional)

Feature request

Description of feature

Example use cases and/or Prototype links

Designs and/or Prototype screenshots

wiredearp commented 6 years ago

We really need some documentation for this, but here it is: Try to call the table.maximize() method. This will make the table expand via position:absolute;top:0;left:0;right:0;bottom:0 so that you must place it in a position:relative container. The Table should now do exactly that: Pin the header while the body is scrolling around. You may need to tweak the page to remove the normal scrollbar, so that this page doesn't show double scrollbars (in any operating system that has scrollbars), at least if it is a "full screen Table". Let us know how that works out. You can see how it works over on http://ui.tradeshift.com/v11/#components/table/demo.html if you click Build Everything to populate the Table with some data.

Chris-Xie commented 6 years ago

But as I tested, maximize would divide the row dataset into multiple pages automatically? It conflicts with our already implemented paging logic. Is there a way to turn it off?

(I tried with table.max(-1), but it doesn't work.)

wiredearp commented 6 years ago

Well, no :cry:

Chris-Xie commented 6 years ago

Well I think maybe auto adjusting rows behavior in maximize method have its usage. But considering this case, the paging is done on the api level - so switching to another page would trigger an api call to fetch that page data. In this case, we don't want to change the row count displayed in each page simply but we would still like to have the capability to scroll table body and keep header pinned.

wiredearp commented 6 years ago

All of that should still be possible when the maximize mode is enabled. If you press the other button "Build Incrementally" over on http://ui.tradeshift.com/v11/#components/table/demo.html, the Table will be build with separate API requests for each "page". There is of course only one page loaded when you do it like this, but you can (and probably already do) manipulate the Pager to make it look like there is more pages (http://ui.tradeshift.com/v11/#components/table/paging.html). When you do it like this - we call that a "Backend-Table" - you remain in charge of the amount of rows displayed.

The demo page in question performs an extra stunt, by the way, although this may not be relevant for you guys: It calculates the optimal amount of rows to display in order for the table to stretch the full height of the window and present a minimal amount of scrolling. For a "backend-Table", this calculation can be complicated because the Pager needs to be synchronized as well (also when you resize), but that code can in any case be seen on http://ui.tradeshift.com/v11/dist/components/table/demo.js if you search for function buildIncrementally. This calculation is all performed via a callback function provided to the maximize(callback) method.

So in short, the Table is not "taking control" of the amount of rows to display. The amount of rows is specifically set by the developer (you). The Table only takes full control when you feed it all the data at once and let the Table handle the Pager; we call that a "clientside Table".

wangcansunking commented 6 years ago

We just want to keep the amount of rows but at the same time we still want to control the height of the table which means table needs to be scrollable with little height and many rows in one page.

wangcansunking commented 6 years ago

Maximize is used to recompute the max-rows of the table. However, it adds scrollable to the table at the same time.

I think scrollable is the character of the table-body according to the container of table and Maximize should only compute the max-rows on resize.

Chris-Xie commented 6 years ago

Here's a demo code snippet: https://codepen.io/anon/pen/oqLygq which captures the effect that we are trying to implement. It contains a few things:

The container wrapping table component has a fixed height of 400px.
Initially, we feed the table with 10 rows to be the data to display as the first page. And we manually configure the pager with 5 pages in total.
We call maximize on the table in order to make it stretch to fit to the container.

As result, we expect to see

table is scrollable (400px is not enough to fit 10 rows) while header is pinned.

However, the actual behavior is that

table is only displaying 8 rows.
only 8 rows is rendered in the html markup.

If I understand your comments above correctly, in this case, we are building "backend-tables" and we haven't done anything special to calculate the optimal amount of rows to display in order for the table to stretch the full height of the window and present a minimal amount of scrolling. So we have no idea what causes the actual behavior we've seen.

We doubt that the calculation mentioned above happens in maximize method.

wiredearp commented 6 years ago

Sorry.

It is a bug.

Chris-Xie commented 6 years ago

We are making some efforts to fix it in this https://github.com/Tradeshift/tradeshift-ui/pull/572.

According to our discussions so far, I suggest that we might take the behavior below:

if the table height is not fixed, we would stretch the whole table component to what it contains. Not scrolling happens inside table component.
otherwise, (e.g. table is absolutely positioned within a relative container with a fixed height / table has been placed in a flexbox layout with overall height fixed), we would make sure the table fit to this height and only table-body is scrollable with all fed rows. This would become the default behavior without the need to call table.maximize().

What do you think?

wiredearp commented 6 years ago

Your approach in CodePen is correct. That is exactly how it is supposed to work. However, there is a bug in the Table: It only shows 8 rows for some strange reason. It should show 10. If that bug is fixed, you can use the Table as it is via the maximize method. Is that correct :question:

The whole point of the maximize method is this: To inform the Table that it is now absolutely positioned within a relative container with a fixed height or that it has been placed in a flexbox layout with overall height fixed. So exactly what you wrote, except the height could also by constrained by Grid layout and what not. This is however hard for the Table to figure out by itself. It would need to parse the CSS and/or the inline styles of itself and the parent element and perhaps an ancestor of the parent element to figure out whether or not it is somehow placed in a fixed height container. This is what the maximize method is supposed to help us with. By calling this method, you can fix the height however you like and the Table will make sure to enable scrolling and expand to fill this height. No need for us to analyze anything.

Now, your suggestion is of course a different. You suggest this to be the default behavior. That is of course fair, but then we just push the problem on to anyone who doesn't use the Table in a fullscreen or "maximized" way. Like this for example.

First Header	Second Header
Content Cell	Content Cell
Content Cell	Content Cell

This Table will now need to analyze the page CSS and the surrounding DOM to infer that it is not supposed to stretch to maximum height and enable a scrollbar unless we introduce a new method notmaximized() that can shortcut this. So I don't think we gain much by making defaulting to the maximized mode, it all depends on how you use the Table. The code snippets on the documentation website all use this second type of Table, for example.

Since the current default behavior is to not stretch to 100% height, that is also why the configuration option scrollable may be confusing. Because being scrollable in itself is not enough: You also need a fixed height. Just like with a normal div with overflow:auto, the scrollbar doesn't actually appear until you also set height:100px. So the maximize method does both, it enables scrolling and sets the height (to the height of the nearest positioned ancestor).

We do of course need some proper documentation for all this.

Chris-Xie commented 6 years ago

Thanks for the explanation. We'll try to fix the maximize method then.

wangcansunking commented 6 years ago

                /**
         * Maximize the layout to fill positioned container.
         * Optionally add resize callback for managing pager.
         * @param @optional {function} onresize
         * @returns {ts.ui.TableSpirit}
         */
        maximize: confirmed('(function)')(
            chained(function(onresize) {
                var ended = gui.BROADCAST_RESIZE_END;
                this.css.add('ts-maximized');
                this.onresize = onresize || this.onresize;
                this.broadcast.add(ended);
                var maxrows = this.max();
                this._createpager();
                if (onresize) {
                    onresize(maxrows);
                }
            })
        )

@wiredearp In the code, maximize will recompute the maxrows and regenerate pager. It is a feature of maximize that you think it is a bug. Could you confirm the behavior of maximize that is just to fill positioned container?

wiredearp commented 6 years ago

Yup, that is definitely the bug right there :+1: Thanks for digging in. I did something to fix that today, but I'm pretty busted with the flu here, so I'm gonna have to continue tomorrow. I'm gonna split this up into two methods:

expand simply maximizes the layout. This is what you need. We'll make sure to support that via an attribute in the HTML so that you don't need to call that method via JS.
optimize can be used to limit the number of rows shown, either automatically or manually based on the information provided via the onresize callback as seen in the code above. This is what you don't want.

The old method maximize will simply call both methods, but it will not be documented once we get to write the documentation for these features. Which we really should, because this bug would have been spotted long ago if this documentation had been written :disappointed:

Chris-Xie commented 6 years ago

Really appreciate it. Will it be fixed in v10?

wiredearp commented 6 years ago

Yes. Did you abort v11?

Chris-Xie commented 6 years ago

v4 apps haven't upgraded with tradeshift-ui@v11 yet. So it would be nice if the fix could be patched to v10.

wiredearp commented 6 years ago

Right. Something came up :disappointed: but we will definitely attempt to fix this bug tomorrow.

Chris-Xie commented 6 years ago

It's working now. much appreciated.

Tradeshift / tradeshift-ui