WindowScroll + Masonry + AutoSizer + CellMeasurer cells are not resized

[kirill-konshin]

I have made a small setup to display a bug when using WindowScroll + Masonry + AutoSizer + CellMeasurer.

The expectation is that cellRenderer will receive proper style with proper width but in fact it always gets defaultWidth:

https://codesandbox.io/s/73921rmy0x

I have fixed it with the line //FIXME NASTY HACK but it think it should be done differently.

[bvaughn]

FWIW, this has nothing to do with WindowScroller or AutoSizer, and it is working as intended. 😄

Masonry passes a cell the width as returned by the CellMeasurerCache . This will always be the defaultWidth because, in the case of the Masonry component, each column has a fixed width (as stated by the docs). Put another way, you're expected to inject a column width into Masonry, not the other way around.

Check out the source code for the Masonry demo page to see how I'm using the component. That demo will show you how you can change column width after initializing, if you want to.

[kirill-konshin]

Yep, I saw that example and I basically did the same thing (but shorter for sake of demo) by making columnWidth accessible via newWidth variable. But this is nasty :) I'd say that if we change columnWidth, then it makes sense to reset the CellMeasurerCache with new value.

[bvaughn]

There is an example of this being done in the code I linked you to...

…

On Sun, Jan 21, 2018 at 1:13 PM Kirill Konshin ***@***.***> wrote: Yep, I saw that example and I basically did the same thing (but shorter for sake of demo) by making columnWidth accessible via newWidth variable. But this is nasty :) I'd say that if we change columnWidth, then it makes sense to reset the CellMeasurerCache with new value. — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#979 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABznb-mcjdLCj3p0-LJuZzqtFzSKQ45ks5tM6hdgaJpZM4Rl9WG> .

[kirill-konshin]

Do you mean this: for now the use of the CellMeasurer render callback's async measure parameter is not supported.? But this makes the whole thing quite useless.

In your demo you use state to store real width. This is a hack. Cache should handle it.

[bvaughn]

Your reply is antagonistic. If you think the component is useless, don't use it. I don't want to debate the usefulness of code I spent time writing and open-sourcing via a GitHub issue.

The documentation clearly states that sync measurements are a constraint of the Masonry component. This was an intentional design decision based on real-world constraints (Pinterest) and offered improved runtime performance. It is clearly mentioned in the docs.

Beyond that, I've provided example code of how to accomplish width changes. There's no reason to re-use the cache vs creating a new one. Either way, all height measurements are invalidated.

[kirill-konshin]

Not trying to underestimate your efforts, this is a great lib!

I was saying that looks like it becomes pointless to call measure function for Masonry because it does nothing.

So the current recommended approach is to ignore measured width and replace it with calculated width from state/var?

I am facing another problem, this probably requires another issue, but let me ask here first. I have added dynamic loading of data to my example https://codesandbox.io/s/73921rmy0x

I export a class Page that uses RenderMasonry component to display "loaded" data (as if RenderMasonry will be connect-ed to Redux, and for some reason images are cropped:

Any window resize fixes the layout.

I found a solution to call onResize after calling measure, but this is very heavy and looks like an overkill.

What would be the best way to fix this?

[bvaughn]

I was saying that looks like it becomes pointless to call measure function for Masonry because it does nothing.

Correct. The CellMeasurerCache is used for both Masonry as well as List/Grid/Table. Masonry is not built to support async measurements. The other three are. That's why the docs list the Masonry constraint. Your follow up question is better suited for Stack Overflow or Slack than GitHub. I suggest asking there!

[kirill-konshin]

It looks more like a bug to me, frankly speaking... if list with images is provided from beginning as part of state, then layout works well. But if it is "loaded" via timeout then I see layout glitch (e.g. masonry is drawn twice, without data and with data). Immediately after window resize it's positioned correctly.

I have double checked the demo, looks like some older version was served, pls take a look https://codesandbox.io/s/73921rmy0x?module=%2Fpages%2Findex.js

[bvaughn]

It looks more like a bug to me, frankly speaking..

That's because you're using the component incorrectly. I have reference the Masonry docs several times in this thread, which clearly say:

Cell measurements must be synchronous. Size impacts layout and async measurements would require frequent layout invalidation. Support for this may be added in the future but for now the use of the CellMeasurer render callback's async measure parameter is not supported.

[kirill-konshin]

I'm sorry, I don't get it. How is it different from CellMeasurer perspective how the data was loaded and images were placed on screen: during initial render or during second render?

If cell measurement is synchronous, then fist case with static data should fail too, images are loaded asynchronously, as a next step after rendering. But it works somehow.

I checked the masonry demo: https://bvaughn.github.io/react-virtualized/#/components/Masonry this is an example of static data.

Do you want to say, that it's impossible to add more images asynchronously (say, load next page)?

[kirill-konshin]

I have found the reason. When images are cached first measurement is successful. With clean cache first render fails the same way as async one.

Documentation should actually in addition to above-mentioned phrase refer here #723 (I came to same approach — calling debounced onResize with cellPositioner.reset() and masonryRef.recomputeCellPositions()).

This approach badly affects the performance, as calculation happens every time when user scrolls (some new images are constantly loaded and unloaded at the top/bottom of viewport). I hope some day this wide-spread use case will receive some official support...

[bvaughn]

I'm going to be honest with you and admit that this discussion has been frustrating for me. I feel that you are simultaneously complaining about missing functionality that the documentation clearly states is not supported while also ignoring my responses pointing this out.

I'm stating this clearly so that you'll know where I'm coming from, in case there has been a misunderstanding. Plain text can be hard sometimes.

I'm going to make one final attempt to answer your question, and after that I'm not going to continue this discussion because I don't think it's productive.

I'm sorry, I don't get it. How is it different from CellMeasurer perspective how the data was loaded and images were placed on screen: during initial render or during second render?

You are welcome to look through the source code to understand this. If you don't want to do that though, you'll just have to accept my word (and the documentation) that this functionality isn't meant to work within the Masonry component because of how it caches position metadata. As I mentioned before, this was an intentional decision, made for the performance gains it provided, and targeting a very specific user/use-case (Pinterest).

I checked the masonry demo: https://bvaughn.github.io/react-virtualized/#/components/Masonry this is an example of static data.

The data is not "static". It's randomized. But it's measurable synchronously (on-mount). If you want to load images, using the Masonry component, you would need to send information to the client about the image aspect ratio so that they could also be measured/calculated synchronously. If that is not possible, then perhaps this is not the correct component for your application to use.

Documentation should actually in addition to above-mentioned phrase refer here #723 (I came to same approach — calling debounced onResize with cellPositioner.reset() and masonryRef.recomputeCellPositions()).

I do not plan to add this to the documentation because it's not going to perform well and I don't have the time or energy to add support for this to the component.

This approach badly affects the performance, as calculation happens every time when user scrolls (some new images are constantly loaded and unloaded at the top/bottom of viewport).

This is specifically why the documentation states that it's not a supported use-case. It's also something I've said, in different words, a couple of times in this thread already.

I hope some day this wide-spread use case will receive some official support...

As with any open source component, you are free to implement this functionality yourself if your application requires it. If you're feeling generous, you can submit it as a PR to this library. Or not. Your call.

[kirill-konshin]

Will you accept a PR with broader explanation (based on the above-mentioned facts) right in Masonry doc page, with links to ticket #723 and links to solutions https://codesandbox.io/s/73921rmy0x?module=%2Fpages%2Findex.js and https://github.com/chux0519/react-virtualized-masonry-async-demo as one of possible ways to solve the issue (with a remark about performance impact)?

[bvaughn]

The CodeSandbox in question is buggy. (Resize the width of the page and the layout breaks for pre-measured cells). If this issue is fixed, I will review and consider a PR like you describe. (I won't promise to accept it until I've reviewed it.)

[kirill-konshin]

You mean when page is enlarged then growing images does not expand cells and get cut even worse than before?

[kirill-konshin]

The only solution for this particular issue, as I see it, is to reset the cache cache.clearAll() and force re-measure of currently visible cells + all before them + preserve scroll position. But how? Any suggestions? Looks like an overkill )

[kirill-konshin]

Still working on a better implementation, can you please suggest the best way to clear the cache and force re-measure of all visible+previous cells? Or for all if it's impossible to determine which are visible? Simple ache.clearAll() does not work, it ruins the layout completely...

Scrolling can be solved via scrollToIndex, I presume.

I already added a simple cache to prevent re-positions when user scroll back and forth causing same images to call onLoad.

[kirill-konshin]

Finally had time for this, and managed to make this work.

I've found in the masonry example the reset method, which calls

cache.cleanAll();
cellPositioner.reset(...things);
masonryRef.clearCellPositions(); // instead of recomputeCellPositions

That's exactly what I was looking for.

https://codesandbox.io/s/6x6qkoprk sandbox updated, bug is gone.

I presume this does not perform well especially if scrolled deep.

[kirill-konshin]

In order to support both resize and unknown image sizes I have implemented the ImagePrefetcher which will first load image and calculate it's size and then pass images along with sizes to renderer, so that Measurer can measure preloaded image with exact size.

• ImagePrefetcher: https://codesandbox.io/s/6x6qkoprk?module=%2Fcomponents%2FImagePrefetcher.js
• Page: https://codesandbox.io/s/6x6qkoprk?module=%2Fpages%2Fprefetched.js

I have figured, that Masonry relies on items order (surprise!) so ImagePrefetcher will expose items preserving the order, breaking on first not-yet-loaded item, which means if item 1 and 3 has been loaded but item 2 was not, only item 1 will be passed.

You can swap to previous mode by changing rendered page in /index.js.

I'm going to release it in NPM, can I send a pull request with updated docs, demo and mention of the package?

[kirill-konshin]

@bvaughn any response?

[kirill-konshin]

@bvaughn I have found the working solution, and I am willing to make a PR but I don't want to waste my efforts so please take a look at it and if it's all good I will make a PR.

[kumarharsh]

@kirill-konshin thanks to your work, I could figure out why my masonry layout was not rendering properly when rendered via toggling a state variable:

Your code had a little typo, but it was good enough to point me into the right direction.

For future explorer (including me) - to reset the Masonry layout on subsequent renders, stick this in your componentWillUpdate():

componentWillUpdate() {
  cache.clearAll();
  cellPositioner.reset({...config});
}

https://codesandbox.io/s/58704l10p