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] First Possible Solution of Masonry Component #27169

Closed
wants to merge 17 commits into from

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Jul 7, 2021

Initial Request for Masonry Component: #17000
Preview of current implementation (2.1.2 CSS Grid with items' heights defined): https://deploy-preview-27169--material-ui.netlify.app/components/masonry/

RFC

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. Alternative solutions

2.1 CSS only

2.1.1 CSS Grid Layout Level 3

masonry feature is provided by CSS Grid
ex) grid-template-rows: masonry, grid-template-columns: masonry.

Currently, it is only available in Firefox (after activating the flag) - https://caniuse.com/mdn-css_properties_grid-template-rows_masonry

This implementation must be complementary, as it is still too early to be supported as only solution.

A+
B+
C+
D+
E+
F+
G+
H+
I+

2.1.2 CSS Grid with items' heights defined

If items' heights are given, there is no need for a JS script to figure out them. We will be able to compute the number of grid row spans for each item. This will use the same approach as 2.2.1 except that this doesn't need a script to figure out the heights. Hence, it will have the same limitation except that there is no need to wait for all items to finish loading in this case.

A- ((1) height of each item needs to be specified by developers, and (2) items' heights are in increments of grid gap.)
B+
C+
D+
E+
F+
G+
H+
I+

2.2 CSS + JavaScript

2.2.1 CSS Grid + Configuring grid-row-end [Current Implementation]

Ref: https://w3bits.com/css-grid-masonry/

  • Workflow: We utilise a Masonry that contains all MasonryItems and a MasonryItem that contains a content (div or img in most of the cases). The height of the content passed to MasonryItem is calculated: contentHeight. totalHeight = contentHeight + spacing, where spacing can be passed to Masonry, should be the height of the MasonryItem containing the content so that it is large enough to display the entire content. Then, the number of row spans for this MasonryItem is computed using the following formula: Math.ceil(totalHeight / spacing). As long as css grid row's height is set to 0 in Masonry, we do not need to worry about it for computation of MasonryItem's height or number of row spans.

A- (items' heights are in increments of grid gap instead of being exactly as large as its content)
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+

It is worth mentioning that compared to the other JavaScript solution (2.2.2), applying setting can be done to items individually and hence there is no need to clone children elements.

2.2.2 - CSS Flexbox + Configuring order

Ref: https://css-tricks.com/piecing-together-approaches-for-a-css-masonry-layout/#order-shifted-elements-in-a-flexbox-layout

  • Workflow: This uses flexbox with direction of column. For each item, the column with minimum height so far should be found and the item is pushed to this column. After all items are pushed to some column, the maximum height of column is computed and this is used as the height of the entire masonry container. Then, lastly, the same order (from 1 to the number of columns) is attached to the elements in the same column, and hence all elements are visually arranged by row.

A+
B+
C+
D+
E+
F-
G-
H-
I+

2.3 Summary

- doesn't indicate "impossible" but rather "limitations".

Requirement CSS Grid Layout Level 3 CSS Grid with items' heights defined CSS Grid + Configuring grid-row-end CSS Flexbox + Configuring order
A + - - +
B + + + +
C + + + +
D + + + +
E + + + +
F + + - -
G + + - -
H + + + -
I + + + +

2.4 Personal Opinion

I think that it is a good idea to provide both CSS only solution and CSS + JS solution. For CSS only solution, "2.1.2 CSS Grid with items' heights defined" is the only option we currently have, and it is expected to be implemented quite quickly as my current implementation resembles this solution a lot. It has a limitation that grid gap between items can look inconsistent with higher spacing, but developers can always fine-tune the little margins of the items they pass to MasonryItem to lessen the inconsistency. For CSS + JS solution, I think "2.2.2 CSS Flexbox + Configuring order" is a better choice than the other JS Solution (current implementation) as "2.2.1 CSS Grid + Configuring grid-row-end" has the same limitation as our potential CSS only solution. Also, I think that the logical tab order issue can potentially be resolved for this solution.

@mui-pr-bot
Copy link

mui-pr-bot commented Jul 7, 2021

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against c0b552d

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.

Looks great for a first iteration 🎉

docs/src/pages/components/masonry/BasicMasonry.js Outdated Show resolved Hide resolved
docs/src/pages/components/masonry/demoData.js Outdated Show resolved Hide resolved
docs/src/pages/components/masonry/masonry.md Outdated Show resolved Hide resolved
docs/src/pages/components/masonry/masonry.md Outdated Show resolved Hide resolved
docs/src/pages/components/masonry/masonry.md 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.d.ts Outdated Show resolved Hide resolved
docs/src/pages/components/masonry/BasicMasonry.tsx Outdated Show resolved Hide resolved
docs/src/pages/components/masonry/BasicMasonry.tsx Outdated Show resolved Hide resolved
@mbrookes

This comment has been minimized.

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
@hbjORbj hbjORbj closed this Jul 9, 2021
@hbjORbj hbjORbj reopened this Jul 9, 2021
@hbjORbj hbjORbj force-pushed the masonry-component branch from 711b1f4 to 6750c6b Compare July 9, 2021 08:19
@hbjORbj hbjORbj changed the title WIP: [Masonry] Build a Masonry Component [Masonry] Build a Masonry Component Jul 12, 2021
@hbjORbj hbjORbj force-pushed the masonry-component branch from 46a27af to 1c4a375 Compare July 12, 2021 13:44
@mnajdova
Copy link
Member

Thanks for the detailed write up and investigation. In my opinion we should start with the CSS only implementation. If I am not mistaken, the only API difference between this and the JS implementation would be that the height of the MasonryItems would need to be defined. In the future we could based on this decide whether we should apply the JS implementation (to automatically calculate heights). cc @mui-org/core

@siriwatknp
Copy link
Member

@hbjORbj If you want to support css grid masonry, there is css feature queries that might help.

@hbjORbj
Copy link
Member Author

hbjORbj commented Jul 15, 2021

I removed all JS codes needed to compute content heights and instead implemented the following solution: 2.1.2 CSS Grid with items' heights defined. Demo can be found in the description above. I will work on writing tests and documentation now and move on to implementing 2.2.2 CSS Flexbox + Configuring order.

Update: Since Limitation A has been resolved for 2.2.1 CSS Grid + Configuring grid-row-end, it is more logical to implement this solution rather than 2.2.2 CSS Flexbox + Configuring order for CSS + JS solution based on the tradeoffs in RFC.

@hbjORbj hbjORbj requested a review from mbrookes July 19, 2021 09:36
@hbjORbj
Copy link
Member Author

hbjORbj commented Jul 19, 2021

One of the main limitations of 2.1.2 CSS Grid with items' heights defined approach, which is that MasonryItem's heights are in increments of grid gap and hence cause visually inconsistent gaps between items (especially when gap is large), has been resolved in this commit: 00d6780.

If this fix is acceptable, based on the tradeoffs in RFC, it makes more sense to implement 2.2.1 CSS Grid + Configuring grid-row-end instead of 2.2.2 CSS Flexbox + Configuring order for CSS + JS solution.

@hbjORbj hbjORbj requested a review from mnajdova July 19, 2021 09:53
@hbjORbj hbjORbj force-pushed the masonry-component branch from ef4f302 to 0cc8d3b Compare July 20, 2021 08:47
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.

Great progress!

packages/material-ui-lab/src/Masonry/Masonry.test.js Outdated Show resolved Hide resolved
packages/material-ui-lab/src/Masonry/Masonry.test.js Outdated Show resolved Hide resolved
packages/material-ui-lab/src/Masonry/Masonry.test.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
packages/material-ui-lab/src/Masonry/Masonry.js Outdated Show resolved Hide resolved
@hbjORbj hbjORbj force-pushed the masonry-component branch 3 times, most recently from a46b76e to 419918e Compare July 20, 2021 20:40
@hbjORbj hbjORbj force-pushed the masonry-component branch from 419918e to c68e5e7 Compare July 20, 2021 21:13
@hbjORbj hbjORbj requested a review from mnajdova July 21, 2021 10:09
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.

Few more comments.

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/MasonryItem/MasonryItem.js Outdated Show resolved Hide resolved
packages/material-ui-lab/src/MasonryItem/MasonryItem.js Outdated Show resolved Hide resolved
@hbjORbj hbjORbj force-pushed the masonry-component branch from ee3bc3a to d8db131 Compare July 21, 2021 15:26
@hbjORbj hbjORbj force-pushed the masonry-component branch from d8db131 to c0b552d Compare July 21, 2021 16:08
@mbrookes
Copy link
Member

Good progress.

We're definitely going to need a solution that doesn't require heights to be defined though. I don't think cropping images with object-fit: cover; will cut it in production, and the size (or aspect ratio) of the content may not even be known in advance in order to to approximate the heights.

For a div, a fixed height is going to mean odd gaps between the content and the bottom of the item at different widths, or perhaps not even fit due to localisation. (Thinking of a card with text for example.)

@siriwatknp
Copy link
Member

We're definitely going to need a solution that doesn't require heights to be defined though

Agree with Matt. Looking from developer experience, defining height for all items is not gonna work.
2.2.2 CSS Flexbox + Configuring order sounds interesting but need to think about some common use case like "infinite scroll" because that might affect performance (if my understanding is correct, all of the items order needs to be recalculated). Also I don't mean that we need to support it but at least we already consider it before making a decision.

I also like https://github.com/paulcollett/react-masonry-css. I think once masonry is available in all browser in the future, it will be like

// at this moment, the item is filled by column
<Masonry>
  <div>My Element</div>
  <div>My Element</div>
  <div>My Element</div>
  ...
</Masonry>

// in the future, developer change the implementation to css grid masonry by
<Masonry autoPlacement>
  <div>My Element</div>
  <div>My Element</div>
  <div>My Element</div>
  ...
</Masonry>

@hbjORbj
Copy link
Member Author

hbjORbj commented Jul 22, 2021

Since Limitation A has been resolved for 2.2.1 CSS Grid + Configuring grid-row-end in this commit 00d6780, I decided to implement this solution rather than 2.2.2 CSS Flexbox + Configuring order for CSS + JS solution as based on the tradeoffs in RFC.

@mnajdova
Copy link
Member

I also like https://github.com/paulcollett/react-masonry-css.

I took a look at and I really like it. The tradeoff they have is that they don't support sorting based on height, and I think it is a reasonable one. @hbjORbj could you check if there is something else missing in their implementation? They do not require height to be known in advance. If this is solved will we need the JS implementation too?

@mbrookes so far we discussed that we need to also support CSS only implementation so that this component can be used in SSR. The idea was that we can do a JavaScript implementation that would be more flexible.

In my opinion if we manage to provide a CSS solution that does not require defining height on elements we should be good with only one implmementation.

@mbrookes
Copy link
Member

mbrookes commented Jul 23, 2021

In the future we could based on this decide whether we should apply the JS implementation
(#27169 (comment))

@mnajdova I took this to mean that the PR could potentially be closed without it.

if we manage to provide a CSS solution that does not require defining height

Unfortunately I don't believe that will be possible until CSS grid masonry is widely supported.

The tradeoff they have is that they don't support sorting based on height

Which we (correctly IMHO) have listed as a "must have". We could certainly have a prop to disable it if the use-case doesn't require it.

@hbjORbj hbjORbj changed the title [Masonry] Build a Masonry Component [Masonry] First Possible Solution of Masonry Component Jul 25, 2021
@hbjORbj
Copy link
Member Author

hbjORbj commented Jul 25, 2021

@siriwatknp @mnajdova the implementation of this benchmark (https://github.com/paulcollett/react-masonry-css) is in fact just a minimal version of 2.2.2 CSS Flexbox + Configuring order with the difference being that the former does not support B. The next item is pushed to the shortest column. [Must-have] while the latter does support it with the use of order. We were trying to find a CSS-only solution to support SSR, but since the suggested benchmark does not achieve that, I think there is no benefit in implementing that as it comes with even more limitations (X for all B, F, G, H).

I think the best option we have is 2.2.1 CSS Grid + Configuring grid-row-end solution as it has the fewest limitations (F, G) among those that do not require pre-defined heights of items.

Implementation of 2.2.1 CSS Grid + Configuring grid-row-end solution which does not require pre-defined heights of items can be found here: #27439. Its preview is here: https://deploy-preview-27439--material-ui.netlify.app/components/masonry/

@oliviertassinari
Copy link
Member

@hbjORbj Great exploration! The current approach (2.1.2) looks really interesting. Items that span multiple columns are easily approachable.

Capture d’écran 2021-07-26 à 00 25 57

Simply set grid-column-end: span 2; and you get it:

Capture d’écran 2021-07-26 à 00 26 18

It seems to be a great candidate baseline to build on top of. For instance, if we add a touch of resize observer on each item we can implement 2.2.1, right? #27439. A demo with this snippet:

var resizeObserver2 = new ResizeObserver(entries => {
  for (let entry of entries) {
    entry.target.parentElement.style.gridRowEnd = `span ${Math.ceil(entry.contentRect.height) + 16}`;
    console.log('ffo', entry.target.parentElement, entry.target.parentElement.style.gridRowEnd);
    console.log('entry.contentRect', entry.contentRect)
  }
});

resizeObserver2.observe($0);
auto-layout_m6bmOlag.mp4

https://deploy-preview-27439--material-ui.netlify.app/components/masonry/. Could this be enough?

@hbjORbj
Copy link
Member Author

hbjORbj commented Jul 27, 2021

In yesterday's meeting, it was decided to go with the implementation in this PR: #27439. Hence, I will close this PR.

@hbjORbj hbjORbj closed this Jul 27, 2021
@oliviertassinari oliviertassinari added the component: masonry This is the name of the generic UI component, not the React module! label Jul 27, 2021
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.

6 participants