seatgeek / react-infinite

A browser-ready efficient scrolling container based on UITableView
Other
2.71k stars 274 forks source link

Variable heights - how to know heights before they get rendered? #61

Open dustingetz opened 9 years ago

dustingetz commented 9 years ago

array_infinite_computer needs to know the heights in advance. I'm building a chat app that doesnt know the height of the messages until they are in the dom. Can I do this without a custom computer? Seems like I could do this with a custom computer since all the computer methods needing height data are only called from life cycle methods after the relevant dom is on the page, has anybody done this?

dustingetz commented 9 years ago

An example with my attempt to implement this is coming

garetht commented 9 years ago

I think this could be done with a custom computer. I haven't implemented the ability to pass a computer into the props yet, but that shouldn't be too difficult.

dustingetz commented 9 years ago

I've implemented this here. It was basically a complete rewrite. The main idea is that heights are always measured out of the DOM, so there is no need for custom computer logic. This is not a complete port and the code needs more factoring. As this was basically a rewrite it is not really suitable for merge. Curious what your thoughts are on my approach and if you have any interest in moving forward somehow with this changeset.

dustingetz commented 9 years ago

I added a messages example - the other examples in this fork are broken and have not been fixed yet.

dustingetz commented 9 years ago

Here's the example hosted: http://jockey-dolly-24536.bitballoon.com/examples/messages.html

garetht commented 9 years ago

@dustingetz I've just looked through it. I very much do like providing the option to allow heights to be measured from the browser if the user wishes, but I'm quite skeptical that getting rid of the computers and tying everything up in the component would be an architectural improvement. Keeping the computers allows computation to be separated from rendering and plugins to be written more easily.

I think the existing computers are important because they provide optimal solutions for different use cases in a way that DOM node computation cannot. For example, when heights are known beforehand the solution computed by the existing computers is always exact. This can be relaxed as necessary if the heights need to be determined by rendering. And probably more minor considerations of memory usage can be improved by specifying the correct computer in the correct circumstances.

I'd be happy to look into making any interface modifications that might be needed to support DOM node height calculation as a computer plugin.

dustingetz commented 9 years ago

Thanks for looking at this changeset. I'd be interested in seeing a computer interface that can support heights that aren't known until render. I ended up going down the path here because, if i am not mistaken, it was not possible to implement this against the computer interface as it existed, but it probably is possible if we change the interface.

FWIW I believe this fork handles all possible cases with one code path - I haven't come up with any cases that would require a plugin to be written. Can you think of any? If the heights are known we can seed measuredHeights from a prop for the exact solution.

garetht commented 9 years ago

@dustingetz I've been wondering about the maxChildren prop you have. I'm wondering how to get rid of that because a sufficiently general DOM measurement solution would not need the user to know anything at all about the heights, but maxChildren suggests the opposite. Perhaps if the children were rendered one at a time until the total exceeded the container height?

I don't doubt that some conditionals can be thrown in to make it work with the current features, but as more features are supported this might grow to be unwieldy. I'm wondering what different logic we might need for two-dimensional grids and for arbitrary flexbox layouts, for example, and if those solutions can be generalized with equal efficiency to all cases. Also, bringing DOM measurements into the component would tie it to specifically DOM elements, while react-infinite very much believes that user interface state and its computation exists as a concept that is completely separable from any particular representation.

dustingetz commented 9 years ago

Thanks for looking again, I greatly appreciate your input! I agree, maxChildren is a hack and can probably be removed.

Thanks for these examples - the 2D case and complicated layout cases are cases I hadn't considered in my design.

I was still unable able to get reverse mode working in v2 design, so am in the middle of v3 redesign. v3 brings back the computer pattern and tracks scrollTop state rather than display index, and computes the entire viewState as a function of scrollTop. It's not quite working yet but I'll ask for more feedback soon.

anthonator commented 9 years ago

@dustingetz I'm running into the exact same issue as you with a chat style app. Is your fork in a state for others to use? Do you plan on contributing it back to react-infinite or are you going to keep it standalone?

dustingetz commented 9 years ago

It is unlikely to be merged back because it's basically a rewrite. I should have a release in the next week.

anthonator commented 9 years ago

@dustingetz did ever create a release for this?

vgoklani commented 9 years ago

@dustingetz you did a really nice job! I would like to use it when you're ready.

dustingetz commented 9 years ago

The code works, it is here: https://github.com/dustingetz/react-infinite

We are not in production yet and there will probably more changes, so this repo should be considered unstable and undocumented.

dilizarov commented 9 years ago

@garetht

I've been thinking... While it is one thing to have dynamic heights, you might be able to simplify the problem by simply having a minimalHeight property instead.

For example, in feeds, I can easily tell you what the minimal height would be for a given element. Knowing this minimalHeight, you can determine the maximum number of elements that could fit into the container. Following this, you'll know that container/minimalHeight = number of elements to keep on DOM. Sure, you may have a few additional elements here and there outside of the viewport, but realistically, that in itself is a huge performance gain.

Assume you have a feed with 10,000 items. With dynamic heights, you might only have 10 items on the DOM. With the minimalHeight, you might have 15~30. Sure, most of those aren't on the screen, but for what it is worth... 15~30 versus 10,000 would still be a huge performance gain, and in fact, I'd think that using minimalHeight would make it a lot easier on the developer.

dustingetz commented 9 years ago

@dilizarov I explored that approach in my fork a couple months ago. You end up needing to know the exact heights of each element anyway to compute which child indexes are visible. If you use an approximation based on minHeight, as you scroll further down the range of visible elements will accumulate error until the visible range is totally bogus.

garetht commented 9 years ago

@dilizarov While not strictly documented, if you want to experiment with minimalHeight you can simply set elementHeight={minimalHeight} and the calculations will be performed in the way you describe. Strange inaccuracies will begin to manifest, but it'll be great if they are within tolerable limits for your use case.

dilizarov commented 9 years ago

@garetht I see. Yeah, I can already see issues. In one of the first posts, you stated a custom computer might be able to handle this. I believe since the computer runs on the data when the element is already on the DOM, one would simply be able to use clientHeight or offsetHeight, right?

clauderic commented 8 years ago

Why not auto-correct the inaccuracies as they manifest themselves during onScroll?

For instance, in my application, every element has a variable height, so I guesstimate the item height, and then during onScroll, I double check that the item height at the current index corresponds to the guesstimated value (if not, I correct the value and update the state accordingly)

eudisd commented 8 years ago

I ran into the exact same problem with a chat app I was building a couple months ago, have not looked again until today. There really should be a way to calculate the heights (or widths for horizontal scroll) based of the DOM calculations.

ykshev commented 8 years ago

I'm stuck with this too and can't understand, why no one still has not built it. It seems so common case.

Radivarig commented 8 years ago

Until some pull requests and issues get closed you can check out https://github.com/Radivarig/react-infinite-any-height

geekyme commented 8 years ago

@Radivarig looks like that approach involves calculation of node heights on mount. There are also cases where each node changes their height throughout their lifespan. Eg. Editing a long text in a chat message to a shorter text.

geekyme commented 8 years ago

I've tried this approach:

1) Set an arbitrary elementHeight based on the average height of my list items 2) After rendering, each child reports its height to the parent via a function passed down as props. This then triggers a rerender of the Infinite list with the correct elementHeight.

This works fairly well except that the DOM manipulations results in the whole list moving up and down. Is that normal ? @garetht

Example: Scrolling down, the DOM operations: 1) Increase the height of the spacer (+150px) 2) Remove 3 nodes that are outside the view port. (Each of height 50px)

This happens even when I hardcoded the heights of each child in elementHeight

Radivarig commented 8 years ago

@geekyme indeed. I'll make heights update on key change

Radivarig commented 8 years ago

@geekyme finally got some time for dynamic height update. Please take a look at this commit

To remount elements pass their indices and update timeStamp in updateHeightsOf prop:

<InfiniteAnyHeight
  updateHeightsOf={{indexList: [], timeStamp: 0}}

When timeStamp changes it updates only elements with indices from indexList. You can also change the element itself as it will recreate it and update its key.

Let me know if this works for you and I'll publish it to npm.

Radivarig commented 8 years ago

Also @geekyme I've noticed that setting scrollTop from script is not working as expected, it jumps right back after a frame so I opened a new issue #175 . The code for correcting these height differences is already there, so after fixing this, moving up/down should be resolved.

nemo commented 8 years ago

Any updates on this one? @Radivarig's version lacks some major features so I'm assuming it's a proof of concept at the moment.

Radivarig commented 8 years ago

Hi @nemo, my version adds a wrapper for height detection and then passes them as heights prop and also passes the rest of the props down. I wouldn't know about the features that are missing, I was unable to use react-infinite in production due to bad scroll container nesting and performance. However if you detect the problem with the wrapper I'll take a look, just open the issue.

avdeev commented 8 years ago

I use react-measure as wrapper for dynamic height calc

heldr commented 8 years ago

react-measure works sweat! Maybe add this to the docs? I can PR if need.

DimitryDushkin commented 8 years ago

Could you provide example with react-measure?

heldr commented 8 years ago
  <Measure whitelist={["height"]}>
    {({ height }) =>
      <Infinite containerHeight={height}>
         <div>My dynamic height</div>
      </Infinite>
    }
  </Measure>
DimitryDushkin commented 8 years ago

Thank you! But I thought it will help measure each element height, because I have variable height for each element.

Anyway I've adopted somehow https://github.com/Radivarig/react-infinite-any-height

alex-mironov commented 7 years ago

@heldr @avdeev probably you can suggest how to calculate Item's height dynamically in parent's component before rendering it?

alex-mironov commented 7 years ago

@heldr it's not quite clear why do you measure container height, seems like we need to measure Items' height

avdeev commented 7 years ago

@alex-mironov you can calculate item heights and save their in state

        <Measure
          key={id}
          onMeasure={(dimensions) => this.onMeasure(id, dimensions)}
          whitelist={['height']}
        >
          <FeedItem id={id} />
        </Measure>

Use state.heights as elementHeight prop of react-infinite

SmboBeast commented 7 years ago

@alex-mironov What is this whitelist prop supposed to be? Could it be you are using an older version of react-measure?

I got this to work (finally) with the newest version of react-measure.