[Masonry] Incorrect number of columns behaviour when items have dynamic height

[jpmtrabbold]

Duplicates

• I have searched the existing issues

Latest version

• I have tested the latest version

Current behavior 😯

If any of the items of the masonry can grow in height, instead of re-accommodate all items to preserve the number of columns, it is creating new columns. Please refer to this sandbox:

Sandbox

In the sandbox demo, if you start opening the accordions, you'll see the undesired columns being created.

Expected behavior 🤔

The masonry should always stick to the number of columns passed in the columns prop.

Steps to reproduce 🕹

columns

Context 🔦

Trying to use the masonry to displays different cards and controls in a dashboard that could have different (and possibly growing) heights.

Your environment 🌎

`npx @mui/envinfo`

System:
    OS: macOS 12.0.1
  Binaries:
    Node: 14.18.0 - ~/.nvm/versions/node/v14.18.0/bin/node
    Yarn: 1.22.15 - ~/.nvm/versions/node/v14.18.0/bin/yarn
    npm: 6.14.15 - ~/.nvm/versions/node/v14.18.0/bin/npm
  Browsers:
    Chrome: 95.0.4638.69 (this browser was used)
    Edge: Not Found
    Firefox: 91.0.2
    Safari: 15.1
  npmPackages:
    @emotion/react: ^11.5.0 => 11.5.0 
    @emotion/styled: ^11.3.0 => 11.3.0 
    @mui/core:  5.0.0-alpha.54 
    @mui/icons-material: ^5.1.0 => 5.1.0 
    @mui/lab: ^5.0.0-alpha.54 => 5.0.0-alpha.54 
    @mui/material: ^5.1.0 => 5.1.0 
    @mui/private-theming:  5.1.0 
    @mui/styled-engine:  5.1.0 
    @mui/styles: ^5.1.0 => 5.1.0 
    @mui/system:  5.1.0 
    @mui/types:  7.1.0 
    @mui/utils:  5.1.0 
    @types/react: ^17.0.34 => 17.0.34 
    react: ^17.0.2 => 17.0.2 
    react-dom: ^17.0.2 => 17.0.2 
    typescript: ^4.4.4 => 4.4.4 

[hbjORbj]

@@ -266,14 +269,14 @@ const Masonry = React.forwardRef(function Masonry(inProps, ref) {
     }
     const container = masonryRef.current;
     if (container && resizeObserver) { 
       // only the masonry container and its first child are observed for resizing;
       // this might cause unforeseen problems in some use cases;
       resizeObserver.observe(container);
       if (container.firstChild) {
         resizeObserver.observe(container.firstChild);
       }
     } 
     return () => (resizeObserver ? resizeObserver.disconnect() : {});
   }, [columns, spacing, children]);

Thanks for the report. When an item's height changes, new computation should run (for new height of Masonry, order and margin of items, etc). The incorrect # of columns comes from an insufficient masonry height because this computation didn't run. It didn't run because resize observer is only attached to the masonry container itself and the first masonry item at the moment. I didn't attach resize observer to every child to account for performance. However, other than this, I can't think of a solid fix.

@siriwatknp @mnajdova any thoughts?

[siriwatknp]

@hbjORbj What if you also observe the line-break elements. If any of the line-break is pushed more than the Masonry's height, then recalculate the height to maintain the columns.

[hbjORbj]

@siriwatknp I've tried that too, but event was still not firing. I think it's because the masonry is set to a fixed height after every computation..

[hbjORbj]

@jpmtrabbold Hi, I opened a PR for this issue! Feel free to have a look.

Here is a fork of your codesandbox with the build of the PR. You can see that the error is gone!
https://codesandbox.io/s/masonry-additional-column-error-forked-i371q?file=/demo.tsx

[jpmtrabbold]

you are brilliant man! solved the problem, with less code :)

[agoblet]

I am experiencing a similar issue as well. From reading this issue and the PR, it might actually be the same issue. My masonry layout is breaking as well. I only have this problem after deploying to production. My tiles have images in there, so the delayed loading might cause the masonry to compute the wrong height before the images are loaded. I have worked around it by changing some state of the masonry parent on image load to trigger the re-render. It would be awesome if this could happen automatically with this PR.

[hbjORbj]

@agoblet Hi, can you replace @mui/lab: ... in your package.json with "@mui/lab": "https://pkg.csb.dev/mui-org/material-ui/commit/1f395898/@mui/lab", and then see if your issue is solved?

[agoblet]

@hbjORbj yes that seems to resolve the issue :) removed the workaround and added that branch, and cannot reproduce the issue on production.

[hbjORbj]

@agoblet @jpmtrabbold
Hi. I would like to let you know that the fix is likely to be merged in the upcoming release, which is on this Monday :)

[agoblet]

Thx for picking this up so quickly again after a period of staleness! @hbjORbj

[jpmtrabbold]

Thank you so much @hbjORbj - legend!