petyosi / react-virtuoso

The most powerful virtual list component for React
https://virtuoso.dev
MIT License
5.25k stars 301 forks source link

Customize the item height lookup #375

Closed stephenh closed 3 years ago

stephenh commented 3 years ago

I have react-virtuoso's List ~90% rendering a CSS grid-based table, but have hit one snag.

Per the customized rendering guide, I'm providing my own List component, which sets display: grid and gridTemplateColumns.

Normally with CSS grid, the cells are supposed to be immediate children of the display: grid.

For react-virtuoso, the Item wrapper div "gets in the way" between the Item and the itemContent return value ... however! a known/kosher way around this is to set the Item's div as display: contents. This kinda "skips" the Item div in the CSS grid's logic, and makes the divs within the Item render as rows / cells really well (each of the rows here comes from itemContent and the column widths are set by the gridTemplateColumn: auto auto auto on the List):

image

The biggest gotcha is that display: contents also makes the offsetHeight of the Item zero (which makes sense, it's getting "skipped" in the DOM), which breaks automatic sizing.

Somewhat sneakily, I can "fix" automatic sizing by changing this line:

    const index = parseInt(child.dataset.index!)
    const knownSize = parseInt(child.dataset.knownSize!)
    // Special change here
    const size = (child.firstElementChild! as HTMLElement)[field]

To look at the Item's child element to get the size.

So, the DOM ends up looking like:

And I'm "skipping" the 2nd level for size measurement, and measuring the cell height.

I know this is somewhat esoteric, but I assert being able to use react-virtuoso to virtualize CSS grids (i.e. without the limitations of Grid) would be pretty great/potentially widely used.

Do you have any thoughts on a List prop that would let me customize that const size = child[field] line to be a function that I could pass in?

So far I'm just running a locally built version and could potentially run this as a fork if we need to, but would be great to have this supported in a kosher way.

Also, happy to learn if I'm missing something obvious about a better way to do CSS grid-based tables in react-virtuoso.

Thanks!

stephenh commented 3 years ago

A way to avoid another prop and "convention-over-configuration" support what I'm trying to do would be something like (in getChangedChildSizes):

    let size = child[field]
    // If size is zero, we might be in a CSS Grid that's using `display: contents`
    // on the Item and/or itemContent or both, so look up to 2 levels down for the 1st cell.
    for (let i = 0, current = child.firstElementChild; i < 2 && size === 0 && current; i++, current = current.firstElementChild) {
      size = (current as HTMLElement)[field]
    }

Which does 2-levels down probing (through Item and itemContent) for the 1st element with a non-zero size, only if the current behavior doesn't find a size (i.e. would be no performance decrease or change in current behavior).

petyosi commented 3 years ago

Hey @stephenh ,

I would happily accept a PR that exposes the height lookup as a prop: (element: HTMLElement) => number. Some people have requested this over the years, I am uncertain on how much that would work, to be honest.

Few things which are worth considering given your approach:

1) Does the ResizeObserver still work with that DOM structure and CSS arrangement? 2) What's the performance? I have tested something similar in the past, and vaguely remember something was not quite right.

Also, you might find this experiment useful - https://github.com/petyosi/react-virtuoso/issues/42#issuecomment-821070316.

stephenh commented 3 years ago

Hey @petyosi , thanks for the quick reply!

I spent ~half an hour adding an itemSize prop, but am stuck not knowing how the urx system works. I intellectually understand the reactive pipe idea, but can't concretely translate that to "okay, how do I achieve x/y/z" yet :-).

Here's what I have so far, and specifically itemSize isn't a valid param to useEmitterValue:

https://github.com/petyosi/react-virtuoso/pull/376/files

I think it's because I changed the size system, and this is in the list system ... which I kinda thought would pull that in, but I guess it isn't?

Per your questions, for resizing, AFAICT it can work but I'm not sure how to fully stress test that, i.e. I've just resized my browser window a few times and it looks fine.

Same for performance, I've got ~few 1000 rows going by and it seems just as fast as other examples, but again disclaimer this is me subjectively scrolling up and down and not a benchmark or what not.

42 is also interesting to see; for us we already have ~good number of "tables but really css grids" in our app that we'd like to seamlessly virtualize, without moving to table tags, and so far it looks like this approach would let us do that.

petyosi commented 3 years ago

I cannot remember for certain if the list system does not export only certain items from the size one. Will check tomorrow. PR looks good otherwise.

github-actions[bot] commented 3 years ago

:tada: This issue has been resolved in version 1.9.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: