[React] Columns are misaligned when using `scrollX: true` in DT2/DT3

choc · July 25

Link to test case:
v2.3.8: https://stackblitz.com/edit/datatables-net-react-simple-9kecywem?file=src%2FApp.tsx
v3.0.0: https://stackblitz.com/edit/datatables-net-react-simple-8wrhohjm?file=src%2FApp.tsx

Description of problem:

when using the options below in DT2/DT3:

options={{
  scrollX: true,
}}

the columns are misaligned.

P.S. You need to make the viewport a bit narrower to see the bug, below approx. 1000px.

allan · July 25

Gah that's frustrating. The scrolling alignment of columns is easily the nastiest part of DataTables. For about 5 months of the v3 development I had an alternative implementation committed, but it meant the scrollbars weren't in the right place (overlapping the header and footer), and decided to roll it back, in part due to that, but also compatibility with some of the extensions.

The issue here is with the slot. Without the slot, there is no alignment problem with scrollX enabled. I might need to expose an API that the React component for DataTables can call when all its rendering is done in order to align the columns.

Many thanks for the test case - leave it with me. I'll take a look next week, which I've left as a kind of "mop up" week.

Allan

allan · July 31

Hi,

Just to say that I haven't forgotten about this! There was a bit more mopping up that I anticipated, so I haven't had a chance to look into this yet, but it is something that I'll do so soon. I'm taking a bit of a break next week, so it will likely be the following week). Similar for the Scroller issue you noted as well.

Allan

lbermudez · August 1

Hoping to get a fix for this as well. If interesting, i'll leave here the fix im using at the moment:

Remove scrollX from the table and:

function installDataTableHorizontalScrollWrapper(table) {
    const node = table.table().node();

    if (
        !node.parentElement ||
        node.parentElement.classList.contains(
            'datatable-scroll-wrapper'
        )
    ) {
        return;
    }

    const wrapper = document.createElement('div');

    wrapper.className = 'datatable-scroll-wrapper';

    node.parentNode.insertBefore(wrapper, node);
    wrapper.appendChild(node);
}

Then,

DataTable.defaults.initComplete = function () {
    const table = this.api();
    installDataTableHorizontalScrollWrapper(table);

};

.datatable-scroll-wrapper {
width: 100%;
overflow-x: auto;
position: relative;
}

allan · August 2

Yes, wrapping it up in a scrolling element is a good option. I actually had just scrollX doing that for about 50% of the development time for DataTables 3, but it caused some compatibility issues with extensions that expected the scrolling to be as it currently is. I might change it longer term, however, the main issue is that I want the scrollbar to be in the tbody - that means using the method it currently does, and all the difficulties that go with it.

Allan

choc · August 27

Hi @allan

Any updates on this? The misalignment issue is really frustrating when using slots, but slots are such an important feature as they make the DataTable much more flexible and allow for richer customization.

allan · August 27

3.0.2 had this commit in it, which fixes the issue, after the first draw. Looking at the example with 3.0.2 installed, unfortunately there is still a bit of misalignment when the table is first draw, which I'll need to dig into.

Allan

allan · August 28

I've just tagged up and released datatables.net-react 1.1.0-beta.1 which has a fix in for this issue. An updated test case is here.

The reason I've made it a beta release is that I've switched from createRoot to createPortal for the slots. I would welcome any testing you can do on the beta so if there are any issues I've missed I can get them ironed out.

Regards,
Allan

choc · August 30

Hi @allan

Thank you so much for this release. So far, I've tested the slot functionality and it works very well. I also tested slots using both column index (including -1) and column name, and both work as expected.

One thing I would like to point out is that autoWidth is true by default, but this can cause a significant performance issue when rendering slots.

For example, I have 30K rows in a table, with the first column rendered using a slot. The table hangs for a very long time because it appears to calculate all rows, even though pagination is enabled by default.
Because of this, my current workaround is to disable autoWidth. After disabling it, the table no longer hangs, as it only renders the rows on the first page.

However, the drawback is that the table does not automatically respond to window/document resizing. For example, if a page contains a sidebar and the sidebar is toggled or hidden, DataTables will not detect the layout change, which can cause the columns to become misaligned.

Therefore, as a workaround, I have to add extra code to call columns.adjust() to fix the misalignment.

Do you think it would be possible to improve this behavior? Is it necessary for autoWidth to calculate all rows regardless of pagination? (It seems to be trying to find the max width for the column?)

Another thing I noticed in the React code is related to TypeScript typing warnings.

Shouldn't this line:

import type { Api as DTApiType, Config as DTConfig } from 'datatables.net';

be:

import type { Api as DTApiType, Options as DTConfig } from 'datatables.net';

after DT v3?

If so, should the corresponding as DTConfig alias also be updated?

Also, for this line:

let DataTablesLib: DTType<any> | null = null;

I got the following warning in the PyCharm IDE:

TS2315: Type DataTable is not generic.

Same warning for this line and this line.

Should it instead be:

let DataTablesLib: DTType | null = null

?

Also, for this line:

    [key: `on${string}`]: Function;

I got the following ESLint warning:

ESLint:

The Function type accepts any function-like value.
Prefer explicitly defining any function parameters and return type.
(@typescript-eslint/no-unsafe-function-type)

The same warning also appears for the create parameter in this line.

For these two lines:

                    name[0]!.toUpperCase() +
                    name.slice(1).replace(/-[a-z]/g, (match) => match[1]!.toUpperCase());

and:

table.current!.ready()

in this line, I got:

ESLint: This assertion is unnecessary since it does not change the type of the expression. (@typescript-eslint/no-unnecessary-type-assertion)

for name[0]! and match[1]!, and table.current!.

Also, would you want to simplify this line?

Replace:

```tsx
table.current.page.info().serverSide === false


 with

```tsx
!table.current.page.info().serverSide

I also got the warning:

Parameter cache is not described in JSDoc

for cache: SlotCache in this line and this line.

There is also this warning:

ESLint: Unnecessary conditional, value is always falsy. (@typescript-eslint/no-unnecessary-condition)

for this line:

        if (!slot) {

Lastly, a few very minor warnings:

ESLint: 'divs' is never reassigned. Use 'const' instead. (prefer-const)
ESLint: 'slot' is never reassigned. Use 'const' instead. (prefer-const)
ESLint: 'result' is never reassigned. Use 'const' instead. (prefer-const)

That's all from my side for now. If I notice anything else, I'll reach out again. Thank you!

allan · August 30

For example, I have 30K rows in a table, with the first column rendered using a slot. The table hangs for a very long time because it appears to calculate all rows, even though pagination is enabled by default.

Make sure that you only return a component for the display data type - e.g.:

<DataTable
    slots={{
        0: (data, type, row) => {
            if (type === 'display') {
                return (
                    <Button onClick={doClick}>
                        Click me!
                    </Button>
                );
            }
            
            return '';
        }
    }}
>

The display data type is requested when drawing that cell. Data for the cell can also be requested for filtering, ordering, etc, but that doesn't need the JSX, so don't use it there, it will hurt performance, as you note. I'll add information about this into the React manual page.

If so, should the corresponding as DTConfig alias also be updated?

Yes it should, thank you.

[key: on${string}]: Function;

Yes, that isn't ideal. At some point I'll write out all of the events and fix this.

I've pushed up a commit for some of the ESLint errors. I haven't changed the "This assertion is unnecessary since it does not change the type of the expression" as I want to check into that further.

Thanks for testing the component out and confirming it is working better now.

Allan

choc · August 31

Hi @allan

Make sure that you only return a component for the display data type

This is exactly the way I used in my testing. Here is a test case showing the performance issues when autoWidth = true (default).

The bug seems more complicated when I thought, rows.add() does not do invalidColumn() but row.add() will. (ref: https://github.com/DataTables/DataTablesSrc/blob/master/js/api/rows.ts and https://github.com/DataTables/DataTablesSrc/blob/master/js/core/data.ts)

Case I: data initialized as [], then became 5000 rows

props.data = [] on init.
autoWidth still run but no rows to loop. wideStrings = [] is cached.
Then, props.data changes, this part is executed: clear() -> rows.add(5000) -> draw(false)
wideStrings is still []. So no need to recalculate width.
only render the first page (10 rows).
console.log("display") x 10.

Case II: 5000 rows from the start
1. props.data = 5000 rows on init.
2. autoWidth will run and loop 5000 rows.
3. console.log("display") x 5000.

I'm not sure whether this is actually a bug that happens to save us from the performance issue.

What would be the best way to balance the performance cost of scanning all rows against the accuracy of autoWidth?

Would calculating the width from only the current page be a reasonable compromise?

BUT, interestingly, the ID column is 71.62px on the first page and 96.51px on the last page for both cases!

The actual rendered column width still changes between pages. I'm not sure what practical benefit is gained from looping every row. In the React case, that initial scan is particularly expensive because it can instantiate thousands of JSX slot renderers.

So, it comes to my last question again: Would it make sense for autoWidth to calculate its initial widths from only the current page, or otherwise avoid invoking expensive display renderers for the entire data?

allan · August 31

If you are adding multiple rows, then yes the plural method is the way to go. What I've done in some other cases (column visibility for example) is include a flag to indicate if the change should be considered "final" and redraw calculations performed. It might be reasonable to include such an option to row.add() and invalidation would only be done when it is "final".

Would it make sense for autoWidth to calculate its initial widths from only the current page, or otherwise avoid invoking expensive display renderers for the entire data?

Yes. Something needs to change there, but I'm not yet certain what! I want the render function to be as low overhead as possible, but rendering JSX for so many cells is going to have significant overhead.

The width calculations really need to be performed over multiple pages of data, in case there is a long string somewhere in the data set, and the whole idea is to make it so the column widths don't bounce around as paging is changed. There is going to be less risk of that when a component is used I think (e.g. a button is more likely to have a fixed width), but there is no guarantee. Possibly there needs to be an extra option. I'll need to mull this over somewhat. Thanks for the test case!

Allan

allan · August 31

I have to say, the flickering of widths in that example is horrible. I really wish there were more hours in the day so I could get all of this stuff done!

[React] Columns are misaligned when using `scrollX: true` in DT2/DT3

[React] Columns are misaligned when using `scrollX: true` in DT2/DT3

Replies