[Masonry] Implement Masonry using Flexbox #28059

hbjORbj · 2021-08-30T14:51:36Z

I have followed (at least) the PR section of the contributing guide.

The current implementation of Masonry relies on CSS Grid. It fails to render all items on Chrome if masonry's height goes beyond 2,000px. This is because Chrome limits the number of grid rows to 1000 at the maximum. Overflowing items beyond the 1000th row are simply squashed.

1. Solution

Replace the CSS grid implementation with one using Flexbox.
Benchmark: https://github.com/gilbitron/flexmasonry
Preview: https://deploy-preview-28059--material-ui.netlify.app/components/masonry/

2. Requirements of `Masonry`

A. The items can have arbitrary heights. [Must-have]
B. The next item is pushed to the shortest column. [Must-have]
C. The items should be ordered by row. [Must-have]
D. The number of columns cols and the spacing between items spacing can be passed into the component. [Should-have]
E. Responsive number of columns (breakpoints) [Nice-to-have]
F. Server side rendering [Nice-to-have]
G. Performance [Nice-to-have]
H. Logical tab order [Should-have]
I. Support for lazy loading/virtualisation [Nice-to-have]

2. Integrity of this implementation

A+
B- (Update: addressed in this commit: dae7e0e and hence +)
C+
D+
E+
F+
G+
H+
I+

B might be able to be resolved as @oliviertassinari said

For solving B, assuming that we measure the height of each item, we can apply for the right order, on our own, proof: https://codepen.io/oliviertassinari/pen/ExXYmyy (see item 5)

It is worth mentioning that the column spanning of the current Masonry will no longer be supported.

mui-pr-bot · 2021-08-30T14:59:22Z

Details of bundle changes

Generated by 🚫 dangerJS against 016703c

oliviertassinari · 2021-08-30T15:04:09Z

We might want to give it more time for developers to test the Masonry (not release yet) in #28059. For instance:

Maybe column spanning is important for developers.
Maybe developers will be happy to trade height precision for supporting the total height
Maybe we can make the height precision resolution automatic based on the total height of the Masonry
Maybe we need two different versions of the Masonry each with its tradeoff, and use the analytics to see which one resonates the most.

eps1lon

Can't say much to the implementation. Just some general question and code nits.

packages/material-ui-lab/src/Masonry/Masonry.js

hbjORbj · 2021-09-01T11:00:34Z

We might want to give it more time for developers to test the Masonry (not release yet) in #28059. For instance:

Maybe column spanning is important for developers.

Maybe developers will be happy to trade height precision for supporting the total height

Maybe we can make the height precision resolution automatic based on the total height of the Masonry

Maybe we need two different versions of the Masonry each with its tradeoff, and use the analytics to see which one resonates the most.

@oliviertassinari Sounds good! I agree that we need feedback from users.

@mnajdova @mbrookes @eps1lon I just wanted to point out that I resolved this limitation B. The next item is pushed to the shortest column. [Must-have] in this commit 😁: dae7e0e

This could be our advantage because most flexbox-based masonry libraries out there (e.g., https://github.com/paulcollett/react-masonry-css, https://github.com/gilbitron/flexmasonry) lack this property, and this becomes problematic especially when items' heights have a wide range because masonry looks unbalanced/broken in such case (ex: https://codepen.io/sldisek783/pen/OJgNgbg).

Anyways, the only known limitation of this implementation compared to the current one using CSS Grid is the column spanning option.

Preview: https://deploy-preview-28059--material-ui.netlify.app/components/masonry/

michaldudak

In general, I find the flexbox approach much more sensible than the grid with gazillion rows :)

docs/src/pages/components/masonry/masonry.md

packages/mui-lab/src/Masonry/Masonry.js

michaldudak · 2021-09-29T19:07:58Z

packages/mui-lab/src/Masonry/Masonry.js

-import PropTypes from 'prop-types';
-import clsx from 'clsx';
+import { unstable_composeClasses as composeClasses } from '@mui/core';
+import { styled, useThemeProps } from '@mui/material/styles';
 import {
  createUnarySpacing,
  getValue,
  handleBreakpoints,


(not related to this PR) This is the first time I encounter these functions and I can't figure out by their names what they are supposed to do. @mnajdova how would you feel about giving them more meaningful names?

Open to ideas, maybe applyBreakpoints is better name :) Let's just make sure it is not a public API before renaming

packages/mui-lab/src/Masonry/Masonry.test.js

hbjORbj · 2021-10-14T10:05:10Z

@hbjORbj Will Masonry support SSR in this PR? If not, I think it is worth mentioning in the docs.

@siriwatknp Yep, I am working on it. :)

packages/mui-lab/src/Masonry/Masonry.js

docs/src/pages/components/masonry/ImageMasonry.js

docs/src/pages/components/masonry/masonry.md

eps1lon · 2021-10-18T10:14:22Z

packages/mui-lab/src/Masonry/Masonry.js

    const container = masonryRef.current;
-    container.addEventListener('scroll', handleScroll);
+    resizeObserver.observe(container);
+    // Observing window in addition to masonry container is more stable


"is more stable" compared to what and in what regard is it more stable?

The use case here is that the masonry's dimensions may not change even if user resizes the window; in case of responsive columns/spacing, when window resizes, @media query responsive values can kick in and configure new width/margin. However, because resize observer doesn't fire any event, computation is not re-run and hence causes a situation like below (CSS expects 5 columns while computation of new order for each item and height for masonry is still for 4 columns). I will add a comment above the code!

@media query responsive values can kick in and configure new width/margin. However, because resize observer doesn't fire any event,

I haven't tested this but reading the MDN page naively, ResizeObserver should fire an event when a media query changes the dimensions of an element:

reports changes to the dimensions of an Element's content or border box

-- https://developer.mozilla.org/en-US/docs/Web/API/ResizeObserver

Could you provide a codesandbox that shows that media queries affecting the content or border box will not trigger ResizeObserver?

@eps1lon I think this may be because a media query changes the dimensions of items inside the masonry rather than the container. If this is the reason, maybe it's a better idea to observe the first child of masonry than to observe the window? Or, my initial idea itself is wrong.

This is a demo prior to adding a listener to the window:
https://codesandbox.io/s/imagemasonry-material-demo-forked-ck9x0

I think this may be because a media query changes the dimensions of items inside the masonry rather than the container. If this is the reason, maybe it's a better idea to observe the first child of masonry than to observe the window?

This sounds like it would also not catch resizes of relevant elements by other means. I'm not sure if this can or cannot happen but the code should clearly document its limitation: It's not about ResizeObserver not firing on media query changes but rather the code not observing all relevant elements.

I see. So, I think I will replace the window listener with the first child (I just tested and bug is not reproducible this way too) and add a comment saying that we only observe the container and the first child.

const container = masonryRef.current; // only the masonry container and its first child are observed for resizing; // this might cause unforeseen problems in some use cases; resizeObserver.observe(container); resizeObserver.observe(container?.firstChild);

Or, do you think we should add every child to the observer?

I don't know enough about the implementation to make that call. How expensive is running the observer callback? How often would that work be wasted when run on every child?

Documenting limitations is a normal thing to do. It's more important to know about limitations than actually fixing them right away.

I don't know enough about the implementation to make that call. How expensive is running the observer callback? How often would that work be wasted when run on every child?

Since we don't know how many items will be passed, observing every child can be very expensive. Also, since we don't yet know any edge case where this limitation is causing a bug, I'd say observing the container and the first child is enough for now.

Documenting limitations is a normal thing to do. It's more important to know about limitations than actually fixing them right away.

Thanks, I am glad to learn that :)

packages/mui-lab/src/Masonry/Masonry.js

michaldudak · 2021-10-20T06:34:57Z

docs/src/pages/components/masonry/SSRMasonry.tsx

+        columns={4}
+        spacing={1}
+        defaultHeight={450}
+        defaultColumns={4}


Why do we need both columns and defaultColumns?

Because SSR can support only column value of number type while columns support both types of responsive value and number. Only with columns, we don't support users who want to have responsive values of columns/spacing but also want to support SSR. I can take some value out of given responsive values of columns/spacing and use it for SSR, but I think this adds unnecessary complexity and user documentation.

packages/mui-lab/src/Masonry/Masonry.js

michaldudak · 2021-10-20T08:00:45Z

packages/mui-lab/src/Masonry/Masonry.js

        }
        // if there is a nested image that isn't rendered yet, masonry's height shouldn't be computed yet
-        child.childNodes.forEach((nestedChild) => {
+        // eslint-disable-next-line


I believe this eslint rule is here for a reason. So you'd have to either use the good old for loop, or (perhaps even preferably now that I think of it) the some method of the array.

I prefer some() method but I need to do Array.from to do that, which contradicts our initial purpose of this change, so I will go with the old for loop here!

mnajdova

👌

hbjORbj force-pushed the flexbox-masonry branch from ab6c06e to ec9b82a Compare August 30, 2021 14:57

hbjORbj added the component: masonry This is the name of the generic UI component, not the React module! label Aug 30, 2021

hbjORbj requested review from mbrookes, mnajdova, oliviertassinari and eps1lon August 30, 2021 15:08

eps1lon requested changes Aug 31, 2021

View reviewed changes

packages/material-ui-lab/src/Masonry/Masonry.js Outdated Show resolved Hide resolved

packages/material-ui-lab/src/Masonry/Masonry.js Outdated Show resolved Hide resolved

packages/material-ui-lab/src/Masonry/Masonry.js Outdated Show resolved Hide resolved

eps1lon reviewed Aug 31, 2021

View reviewed changes

packages/material-ui-lab/src/Masonry/Masonry.js Outdated Show resolved Hide resolved

hbjORbj force-pushed the flexbox-masonry branch from 3af341e to 75006ec Compare September 1, 2021 08:01

eps1lon reviewed Sep 1, 2021

View reviewed changes

packages/material-ui-lab/src/Masonry/Masonry.js Outdated Show resolved Hide resolved

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 1, 2021

hbjORbj force-pushed the flexbox-masonry branch from 0b90478 to 14e3341 Compare September 1, 2021 10:54

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 1, 2021

github-actions bot mentioned this pull request Sep 1, 2021

Update snapshots for pr/28059 eps1lon/mui-scripts-incubator#3035

Closed

eps1lon approved these changes Sep 2, 2021

View reviewed changes

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 2, 2021

hbjORbj force-pushed the flexbox-masonry branch from d4bfae6 to df21ffd Compare September 2, 2021 13:25

github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Sep 2, 2021

eps1lon changed the base branch from next to master September 14, 2021 09:17

This was referenced Sep 27, 2021

[website] Add components index page #28485

Merged

[Masonry] Items at the bottom of component fail to be rendered on Chrome #27934

Closed

hbjORbj force-pushed the flexbox-masonry branch from df21ffd to 7ac8250 Compare September 27, 2021 14:29

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 27, 2021

hbjORbj requested a review from michaldudak September 29, 2021 09:12

michaldudak reviewed Sep 29, 2021

View reviewed changes

hbjORbj mentioned this pull request Oct 1, 2021

Add a Masonry Component #17000

Closed

1 task

siriwatknp reviewed Oct 14, 2021

View reviewed changes

hbjORbj added 3 commits October 16, 2021 09:53

[Masonry] Refactor and allow item with nested children render well

a360f1b

[Masonry] Improve image masonry demo

bf2c521

[Masonry] Support server side rendering

05c406c

hbjORbj force-pushed the flexbox-masonry branch 2 times, most recently from e5dfb93 to cf44711 Compare October 18, 2021 08:59

[Masonry] Add comments

976db5a

hbjORbj force-pushed the flexbox-masonry branch from cf44711 to 976db5a Compare October 18, 2021 09:16

hbjORbj requested review from siriwatknp, eps1lon and michaldudak October 18, 2021 09:30

eps1lon requested changes Oct 18, 2021

View reviewed changes

michaldudak reviewed Oct 20, 2021

View reviewed changes

[Masonry] Address code review comments

b3242e6

michaldudak reviewed Oct 20, 2021

View reviewed changes

hbjORbj added 3 commits October 20, 2021 09:37

[Masonry] Add more comments

58332af

[Masonry] Add more test

83ef6cd

[Masonry] Fix test

cd67243

hbjORbj force-pushed the flexbox-masonry branch from 0d54459 to 9341378 Compare October 20, 2021 10:09

[Masonry] Set width to 100%

55affcb

hbjORbj force-pushed the flexbox-masonry branch from 9341378 to 55affcb Compare October 20, 2021 10:24

[Masonry] Observe the first child of masonry rather than window

8463cd2

hbjORbj force-pushed the flexbox-masonry branch from c449262 to 8463cd2 Compare October 20, 2021 10:43

[Masonry] Fix build

016703c

hbjORbj force-pushed the flexbox-masonry branch from 1f6522e to 016703c Compare October 20, 2021 12:18

mnajdova approved these changes Oct 20, 2021

View reviewed changes

hbjORbj merged commit 0ff9cb5 into mui:master Oct 20, 2021

eps1lon mentioned this pull request Oct 28, 2021

[Masonry] Fix crash on unmount when using React 18 #29358

Merged

hbjORbj mentioned this pull request Nov 3, 2021

[Masonry] Check if container or child exists to prevent error #29452

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Masonry] Implement Masonry using Flexbox #28059

[Masonry] Implement Masonry using Flexbox #28059

hbjORbj commented Aug 30, 2021 •

edited

Loading

mui-pr-bot commented Aug 30, 2021 •

edited

Loading

oliviertassinari commented Aug 30, 2021 •

edited

Loading

eps1lon left a comment

hbjORbj commented Sep 1, 2021

michaldudak left a comment

michaldudak Sep 29, 2021

mnajdova Sep 30, 2021 •

edited

Loading

hbjORbj commented Oct 14, 2021

eps1lon Oct 18, 2021

hbjORbj Oct 20, 2021 •

edited

Loading

eps1lon Oct 20, 2021 •

edited

Loading

hbjORbj Oct 20, 2021

eps1lon Oct 20, 2021 •

edited

Loading

hbjORbj Oct 20, 2021 •

edited

Loading

hbjORbj Oct 20, 2021 •

edited

Loading

eps1lon Oct 20, 2021

hbjORbj Oct 20, 2021 •

edited

Loading

michaldudak Oct 20, 2021

hbjORbj Oct 20, 2021

michaldudak Oct 20, 2021

hbjORbj Oct 20, 2021

mnajdova left a comment

[Masonry] Implement Masonry using Flexbox #28059

[Masonry] Implement Masonry using Flexbox #28059

Conversation

hbjORbj commented Aug 30, 2021 • edited Loading

1. Solution

2. Requirements of Masonry

2. Integrity of this implementation

mui-pr-bot commented Aug 30, 2021 • edited Loading

oliviertassinari commented Aug 30, 2021 • edited Loading

eps1lon left a comment

Choose a reason for hiding this comment

hbjORbj commented Sep 1, 2021

michaldudak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

hbjORbj commented Oct 14, 2021

Choose a reason for hiding this comment

hbjORbj Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

eps1lon Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eps1lon Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

hbjORbj Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

hbjORbj Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hbjORbj Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnajdova left a comment

Choose a reason for hiding this comment

hbjORbj commented Aug 30, 2021 •

edited

Loading

2. Requirements of `Masonry`

mui-pr-bot commented Aug 30, 2021 •

edited

Loading

oliviertassinari commented Aug 30, 2021 •

edited

Loading

mnajdova Sep 30, 2021 •

edited

Loading

hbjORbj Oct 20, 2021 •

edited

Loading

eps1lon Oct 20, 2021 •

edited

Loading

eps1lon Oct 20, 2021 •

edited

Loading

hbjORbj Oct 20, 2021 •

edited

Loading

hbjORbj Oct 20, 2021 •

edited

Loading

hbjORbj Oct 20, 2021 •

edited

Loading