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

[Masonry] Add new component #27439

Merged
merged 59 commits into from
Aug 25, 2021
Merged

[Masonry] Add new component #27439

merged 59 commits into from
Aug 25, 2021

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Jul 25, 2021

A continuation of #27169
Preview: https://deploy-preview-27439--material-ui.netlify.app/components/masonry/
Closes #17000

1. Problem

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. Solution

Using CSS Grid + Configuring grid-row-end

A+
B+
C+
D+
E+
F-
G- (All items must finish loading in order to do computation and apply grid spans to MasonryItems. Currently, I hide all items until document fully loads everything. I think it is a good idea to accept a loader prop from user and display this loader instead of just hiding items. )
H+
I+

@mui-pr-bot
Copy link

mui-pr-bot commented Jul 25, 2021

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against 3bfc972

@hbjORbj
Copy link
Member Author

hbjORbj commented Jul 27, 2021

@oliviertassinari I addressed all your comments in #27169 in this commit of this PR: d32d3d5

One thing worth mentioning is that since the images used in https://deploy-preview-27169--material-ui.netlify.app/components/image-list/#standard-image-list all have the same height, they aren't sufficient to showcase the masonry component. So I mixed the current image set with those used in ImageList.

@oliviertassinari oliviertassinari added the component: masonry This is the name of the generic UI component, not the React module! label Jul 27, 2021
@oliviertassinari oliviertassinari changed the title [Masonry] Second Possible Solution of Masonry Component [Masonry] Add new component Jul 27, 2021
package.json Outdated Show resolved Hide resolved
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs page is frozen. First, probably because the images we are loading are likely x10 larger than we need. to, but I also suspect something else completely off with the resize-observer polyfill we are using.

docs/src/pages/components/masonry/BasicMasonry.js Outdated Show resolved Hide resolved
docs/src/pages/components/masonry/BasicMasonry.js Outdated Show resolved Hide resolved
@hbjORbj
Copy link
Member Author

hbjORbj commented Jul 27, 2021

@oliviertassinari I just compared two versions: one using resize event listener and the other using ResizeObserver. I think using resize event is much faster. With ResizeObserver, as in the current demo, when you resize the window, the delay is too long.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 27, 2021

I just compared two versions: one using resize event listener and the other using ResizeObserver. I think using resize event is much faster. With ResizeObserver, as in the current demo, when you resize the window, the delay is too long.

@hbjORbj I think that what's important is that if the content of MasonryItem changes, changing its height, then the masonry layout updates, e.g. an image that finishes loading, changing the height of the item. The window resize event won't help much here.

docs/src/pages/components/masonry/ImageMasonry.js Outdated Show resolved Hide resolved
docs/src/pages/components/masonry/ImageMasonry.js Outdated Show resolved Hide resolved
docs/src/pages/components/masonry/ImageMasonry.js Outdated Show resolved Hide resolved
docs/src/pages/components/masonry/ImageMasonry.js Outdated Show resolved Hide resolved
docs/src/pages/components/masonry/ImageMasonry.js Outdated Show resolved Hide resolved
@hbjORbj hbjORbj force-pushed the masonry-component-v2 branch from d32d3d5 to 9d8596b Compare July 27, 2021 22:35
@hbjORbj
Copy link
Member Author

hbjORbj commented Jul 27, 2021

@oliviertassinari I allowed height prop so that user can enjoy SSR as well and added a demo of this. I refactored ResizeObserver a bit and optimised images in demos for less loading time. Please let me know if image optimisation can be improved because I am not so sure if I have done the right things.

@hbjORbj hbjORbj force-pushed the masonry-component-v2 branch 3 times, most recently from 679d3d8 to fefd78a Compare July 28, 2021 13:17
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resize observer logic doesn't seem to work:

empty.spot.mp4

Otherwise, it seems that we should focus on the documentation next, and I guess we are done?

packages/material-ui-lab/src/MasonryItem/MasonryItem.js Outdated Show resolved Hide resolved
packages/material-ui-lab/src/MasonryItem/MasonryItem.js Outdated Show resolved Hide resolved
packages/material-ui-lab/src/Masonry/index.js Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
docs/src/pages/components/masonry/ImageMasonry.tsx Outdated Show resolved Hide resolved
@hbjORbj hbjORbj force-pushed the masonry-component-v2 branch from ed5c404 to cec63ac Compare July 28, 2021 15:25
@hbjORbj hbjORbj force-pushed the masonry-component-v2 branch from e2dab00 to 7a4ddbd Compare July 28, 2021 22:29
@hbjORbj
Copy link
Member Author

hbjORbj commented Jul 28, 2021

@oliviertassinari Here is a fix for correctly using ResizeObserver! 7a4ddbd

Fix.Resize.mov

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't spot any issues with the behavior, works great. Some comments for polishing, apart from that looks good 🎉

docs/src/pages/components/masonry/masonry.md Outdated Show resolved Hide resolved
docs/src/pages/components/masonry/DiffColSizeMasonry.tsx Outdated Show resolved Hide resolved
packages/material-ui-lab/src/Masonry/Masonry.d.ts Outdated Show resolved Hide resolved
packages/material-ui-lab/src/Masonry/Masonry.d.ts Outdated Show resolved Hide resolved
packages/material-ui-lab/src/Masonry/Masonry.js Outdated Show resolved Hide resolved
packages/material-ui-lab/src/MasonryItem/MasonryItem.js Outdated Show resolved Hide resolved
packages/material-ui-lab/src/MasonryItem/MasonryItem.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@mnajdova
Copy link
Member

mnajdova commented Jul 29, 2021

@oliviertassinari how can we improve the visual regresssion tests: https://www.argos-ci.com/mui-org/material-ui/builds/149? They are not really useful at the moment.

@hbjORbj hbjORbj force-pushed the masonry-component-v2 branch from 2965f4c to d9afcac Compare July 29, 2021 12:15
@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 29, 2021

how can we improve the visual regression tests?

@mnajdova First, I would make docs-components-masonry/ImageMasonry.png and any demo with images ignored:

https://github.com/mui-org/material-ui/blob/75f27074b89381d25a1fab85fc2098af58e89cb0/test/regressions/index.js#L24

Second, I would add a min-width: 300px; (it's currently ~100px, just enough to stay below the mobile view width) to the pure layout demos.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 29, 2021

Here is a fix for correctly using ResizeObserver! 7a4ddbd

@hbjORbj Awesome, now we can focus on the documentation. I would personally propose something in this order, based on the survey, the issue, and the benchmark:

# Masonry

## Basic masonry

basic demo

## Image masonry

image demo

Mention it's an alternative to https://deploy-preview-27439--material-ui.netlify.app/components/image-list/#masonry-image-list. pros vs. cons

## Spacing

demo

## Responsive values

demo

## Column spanning

demo

## Variable column widths

demo

## Server-side rendering

demo

@hbjORbj hbjORbj force-pushed the masonry-component-v2 branch from f59c1c3 to c163bb2 Compare August 24, 2021 16:17
@mbrookes
Copy link
Member

By passing propName to <Component />, you can configure...

I don't believe we use this format for other components. Please consider standardising - either change this component to match the others, or all others to match this 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: masonry This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a Masonry Component
9 participants