Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Collections with dynamic heights #563

Closed
chrislloyd opened this issue Feb 4, 2017 · 6 comments
Closed

Collections with dynamic heights #563

chrislloyd opened this issue Feb 4, 2017 · 6 comments

Comments

@chrislloyd
Copy link

As mentioned in person to @bvaughn, I'd like to start using React Virtualized for laying out collections of items with dynamic heights. While Collection is super useful, the user needs to know the width and height of a cell before rendering it, which is difficult when cells contain text. What does this mean practically? Well I'd love to see/build something like this:

<Collection
  cellRenderer={({ index, isScrolling, isVisible, key, style }) => (
    <div key={key} style={{
      ...style,
      ...(isVisible ? { display: 'none' } : {}),
    }>{index}</div>
  )}
  cellSizeAndPositionGetter={({ index, width, height }) => {
    return { top: 100, left: 100 };
  }}
/>

Basically, instead of the width and height having to be calculated by the caller, the Collection can render the item, measure it's width and height, then pass that to cellSizeAndPositionGetter. From there, the cellRenderer just needs to be away of wether the cell is in view or not and can toggle display: none appropriately. React Virtualized would need to render all the items (preferably in batches) and leave them in the dom. However, if they are display: none'd when not visible, they won't affect reflow perf.

Another thing to consider is that server rendering these Collections is impossible. In that situation it might be worthwhile providing an extra property to the cellRenderer like isMeasuring so that clients could specify a server fallback. As an example taking the layout from the RV Collection docs, clients could float position cells so that the first line is correctly positioned server side.

One alternative interface would be to add display: none to the styles prop passed to the cellRenderer so that users wouldn't need to worry about it themselves, however that gives less flexibility in how the styles are applied (it would be more efficient to use CSS selectors rather than inline).

This is all a bit of a brain dump about the topic, but I'd like to gage the interest for something like this in Virtualized and see wether the approach is 🆗 before committing to a PR :)

@bvaughn
Copy link
Owner

bvaughn commented Feb 5, 2017

Hey @chrislloyd. Thanks for sharing these thoughts. 😄

One of the tricky things about what you're proposing is that- without knowing the width/height of a given cell, Collection would not know how many it needed to measure. Starting at 0,0 would be pretty easy because it would only need to measure cells with x/y in the range of 0,0...width,height. But if a user scrolled (via props) to X,Y then Collection would need to account for all of the cells with x/y in the new visible range as well as all cells to the left and above that might extend into that range. There are heuristics we could use to limit this impact, eg max-width and max-height, but we'd have to be careful not to sync-measure an entire collection because that would crush initial performance.

React Virtualized would need to render all the items (preferably in batches) and leave them in the dom. However, if they are display: none'd when not visible, they won't affect reflow perf.

Reflow is one perf consideration but not the only one. Every time Collection changes- it will render- which would need to create N (basically throw-away) React elements to then be diff'ed (even efficiently, it's still O(n)) in order to maintain those display: none DOM elements. Plus there's the overhead of creating/maintaining that many DOM elements. I think this is not a route we'd want to go down TBH.

@chrislloyd
Copy link
Author

chrislloyd commented Feb 5, 2017

First off, thanks for taking the time to reply — this is very exciting!

There are heuristics we could use to limit this impact, eg max-width and max-height

Yep — I'm not really interested in trying to follow those either. It'll just end up too messy.

we'd have to be careful not to sync-measure an entire collection because that would crush initial performance.

We currently batch the pre-rendering of the cells to split that cost across multiple frames. It does create some scroll jerk but it tends to not be a problem as it's in line with what users are seeing visually. Though I haven't played around with Fiber too much, I'm hoping that will help with this problem eventually.

I think this is not a route we'd want to go down TBH.

I think it's a tradeoff. We're currently employing a technique similar to this (albeit not using React) and real world perf is OK. n is generally < 1k and updates to Collection happen infrequently.

We'd obviously love to utilize RV, so a few possible approaches come to mind:

  1. Add a switch to Collection that toggles it's behavior while suitably informing users of it's significant tradeoffs. <Collection slowlyMeasureCellSize ... />
  2. Add a new top level container component with a similar API but with the appropriate documentation and warnings. <SlowlyMeasuredCollection ... />
  3. We start with Extremely slow scrolling with scroll wheel/arrow keys in Firefox #2 but keep the implementation private and meanwhile help solidify the virtualized component interface so we can keep compatibility with the RV HoCs. interface Virtualizable<T> { ... }

Cheers!

@bvaughn
Copy link
Owner

bvaughn commented Feb 5, 2017

There are heuristics we could use to limit this impact, eg max-width and max-height

Yep — I'm not really interested in trying to follow those either. It'll just end up too messy.

We might be misunderstanding each other. I think it's important that such a component as we're discussing not have to measure every cell in it before laying out the first chunk of cells. Sounds like your use-case is < 1k items but we shouldn't expect that to be the case for other users. RV components are regularly used with 10s or 100s of thousands of items. 😁

Fortunately I don't think it would be that complicated to implement either.

@bvaughn
Copy link
Owner

bvaughn commented Feb 5, 2017

Here's a rough sketch of what I had in mind:

const { maxCellHeight, maxCellWidth } = this.props
const { scrollLeft, scrollTop } = this.state

const viewportRight = scrollLeft + width
const viewportBottom = scrollTop + height

// Items that may be visible in the currently scrolled viewport.
// Perhaps we could short-circuit this filter by tracking filtered coordinates.
const filteredItems = items.filter((item) => {
  const id = item.id // Over-simplification :)

  let position;
  if (cachedPositions.hasOwnProperty(id)) {
    position = cachedPositions[id]
  } else {
    cachedPositions[id] = position = cellPositionGetter({ index })
  }

  if (
    isVisible({
      left: position.left,
      top: position.top
    })
  ) {
    return true
  }

  const maybeVisible = isVisible({
    bottom: position.top + maxCellHeight,
    left: position.left,
    right: position.left + maxCellWidth,
    top: position.top
  })

  // Depending on how big maxCellHeight/maxCellWidth are,
  // We may not even need to ACTUALLY measure the items.
  // It might be okay to just render them anyway.
  // But if we need to, we can further filter.
  if (maybeVisible) {
    const measurement = cachedMeasurements.hasOwnProperty(id)
      ? cachedMeasurements[id]
      : measure(item) // CellMeasurer hook

    return isVisible({
      bottom: position.top + measurement.height,
      left: position.left,
      right: position.left + measurement.width,
      top: position.top
    })
  }

  return false
})

Something like this ^ should allow us to defer actually rendering and measuring cells until they are likely to actually be displayed.

@bvaughn
Copy link
Owner

bvaughn commented Feb 8, 2017

Hey @chrislloyd! Unfortunately I'm not going to make it to React Wed tomorrow. Something came up that I need to take care in the afternoon near home. But I'd love to talk more about this at some point. Perhaps we could do a Hangout or Messenger video chat. 😄

@bvaughn
Copy link
Owner

bvaughn commented Mar 6, 2017

This issue isn't really actionable without some further discussion- which we have planned for the next React Wednesday and/or during React Conf- so I'm going to close this issue for now as I don't like to leave non-actionable issues hanging around.

Let's talk more about this in person! We can open issue(s) as needed once we decide on a plan.

@bvaughn bvaughn closed this as completed Mar 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants