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 experimental support for two-column items #2955
Masonry: add experimental support for two-column items #2955
Conversation
✅ Deploy Preview for gestalt ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
// Combine winning positions and 2-col item position, add to cache | ||
const finalPositions = [ | ||
...winningPositions, | ||
{ item: twoColItem, position: twoColItemPosition }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thinking ahead, once we integrate this into Pinboard it would be good to somehow have access to this so we can log data on average number of whitespace we end up with per pass. maybe something like a callback prop could help with that e.g. onTwoColumnLayout(position, heights)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was thinking about that as well. I think I'll add that in a future PR so we can start playing around with these changes in Pinboard
const layoutNumberToCssDimension = (n: ?number) => (n !== Infinity ? n : undefined); | ||
// When there's a 2-col item in the most recently fetched batch of items, we need to measure more items to ensure we have enough possible layouts to find a suitable one | ||
// The number of items measured at a time is the number of columns * this multiplier | ||
const TWO_COL_ITEMS_MEASURE_BATCH_SIZE = 6; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a comment around why this is static and why we're doing 6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good call, I didn't update that comment
*/ | ||
renderItem: ({| | ||
+data: T, | ||
+itemIdx: number, | ||
+isMeasuring: boolean, | ||
+heightAdjustment?: number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this atm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'd left a comment explaining and then forgot to submit 🤦 We don't need it yet, so it could be removed. This will be used for future pin leveling work.
packages/gestalt/src/Masonry.js
Outdated
|
||
const itemsWithoutPositions = items.filter((item) => item && !positionStore.has(item)); | ||
// $FlowFixMe[prop-missing] We're assuming `columnSpan` exists | ||
const hasTwoColumnItems = itemsWithoutPositions.some((item) => item.columnSpan === 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const hasTwoColumnItems = _twoColItems && itemsWithout....
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
numberOfItems?: number, | ||
previousItemCount?: number, | ||
randomNumberSeed?: number, | ||
twoColItems?: boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is new, the rest is alphabetizing
color: string, | ||
columnSpan?: number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is new, the rest is alphabetizing
numberOfItems = 20, | ||
previousItemCount = 0, | ||
randomNumberSeed = 0, | ||
twoColItems, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is new, the rest is alphabetizing
numberOfItems?: number, | ||
pinHeightsSample: $ReadOnlyArray<number>, | ||
previousItemCount?: number, | ||
randomNumberSeed?: number, | ||
twoColItems?: boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is new, the rest is alphabetizing
numberOfItems = 20, | ||
pinHeightsSample, | ||
previousItemCount = 0, | ||
randomNumberSeed = 0, | ||
twoColItems, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is new, the rest is alphabetizing
*/ | ||
renderItem: ({| | ||
+data: T, | ||
+itemIdx: number, | ||
+isMeasuring: boolean, | ||
+heightAdjustment?: number, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be for future pin leveling support. Could possibly be removed for now, not used yet
* | ||
* This is an experimental prop and may be removed in the future. | ||
*/ | ||
_twoColItems?: boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Props prefixed with _
are automatically filtered out of the autogen Props tables in the docs
@@ -201,6 +179,52 @@ export default class Masonry<T: { ... }> extends ReactComponent<Props<T>, State< | |||
}; | |||
} | |||
|
|||
containerHeight: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of these changed, just organizing
|
||
type Edge<T> = {| | ||
score: number, | ||
// eslint-disable-next-line no-use-before-define |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is circular, can't be avoided
@@ -1,7 +1,6 @@ | |||
// @flow strict | |||
import { type Cache } from './Cache.js'; | |||
|
|||
// replace V with "number" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This old note to myself was wrong, I needed to fix the generic type used in Masonry
Summary
This PR introduces a proof-of-concept to support rendering 2-column modules in the Masonry grid. See the project doc for a comprehensive summary of the project.
This PR adds an undocumented experimental prop to Masonry. When this prop is
true
(and usinglayout
is"basic"
or"basicCentered"
, Masonry will use a new layout algorithm. When a batch of pins to be laid out contains a 2-col module, we switch from the classic layout algorithm ("put the item in the shortest column") to a graph-based approach. This approach takes inspiration from the seminal Knuth+Plass text layout algorithm for TeX to explore every possible layout using the 1-col items in the batch, looking for the layout that will yield two adjacent columns of very similar height. The 2-col module will be inserted in this location, and the remaining 1-col items will be laid out normally.A major conceptual change for this is that when using the new algorithm, we cache the column heights array and item positions (by item data reference). In the classic algorithm, we throw away heights and positions on each render and lay out the entire grid from scratch each time. This depends on a major assumption: that grid items will always be laid out in the order given. Since we now break that assumption in the batch of items containing the 2-col module we need to be able to directly associate position with a specific item, not just an
items
index.This PR does not yet implement pin leveling to minimize whitespace above the 2-col module. That technique may not be necessary for an MVP for this project — and if it is, it can be implemented in a future PR.
Testing these changes
Since this is an experimental POC, the changes are not represented on the Masonry docs page. Here's how to test locally:
yarn start
http://localhost:8888/integration-test/masonry?realisticPinHeights=1&twoColItems=1
(realisticPinHeights
is optional)Demo
Screen.Recording.2023-06-22.at.2.47.18.PM.mov
A note about this demo: this is using the "realistic pin heights" option introduced back in #2823. Though based on actual pin data, the "realism" here is questionable. There seems to be a lot more variation in heights than I've ever seen in a grid. So the whitespace above 2-col modules is possibly (probably?) worse than it will be in a "real world" grid.