-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: add maxSpace option to @react-stately/layout GridLayout #8654
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
feat: add maxSpace option to @react-stately/layout GridLayout #8654
Conversation
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 seems reasonable to me, sounds like a good idea to add to other applicable layouts (Waterfall). Not a call to action though, we can focus on just GridLayout for this PR.
/** | ||
* The maximum allowed horizontal space between items. | ||
* @default Infinity | ||
*/ | ||
maxSpace?: 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.
should this perhaps mirror minSpace
by also determining the maximum vertical spacing between the items in the Grid?
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.
I thought about it (and happy to do it), but currently minSpace
height value is effectively used as the vertical spacing value (not minimum nor maximum), so I wouldn't know what to do with the added maxSpace.height
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, it certainly does feel a bit silly since the vertical spacing is always determined by the user unlike the horizontal spacing which depends on the item size/container width etc. Off the top of my head, I'd imagine we either change the name of maxSpace
to reflect that it only applies for the horizontal spacing OR we keep the name and have it take it take a Size
for consistency.
Leaning towards the first one personally if only to avoid having to do Math.min(maxSpace.height, minSpace.height)
(again, silly because the user provides both)
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.
I renamed the parameter maxHorizontalSize
Also, would you mind signing the CLA? See the CI check below |
Yep, I'm on it with legal on my side 😉 |
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.
Thanks for signing the CLA! Things look good to me, will see about getting another pair of eyes on this
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.
LGTM, thanks for the thorough description!
Closes #8655
✅ Pull Request Checklist:
📝 Test Instructions:
Run the storybook (locally)
Under "React Aria Components > GridList > Virtualized Grid List Grid"
maxSpace
)maxSpace: 5
)you can also test for more complex combination by modifying the
layoutOptions
.For example with
layoutOptions
:maxSpace
)maxSpace: 5
)🧢 Your Project:
Algolia Visual Editor is available in the Algolia Dashboard & allows to create Rules to curate results for specific queries.
It makes use of Drag & Drop capabilities to allow to "pin" records to specific position (see example around 1min20s in this video)
we're currently in the process of migrating our existing drag & drop library (and virtualizer) to use
react-aria-components
.In order to be able to consistently render grids of records across many pages in the Algolia Dashboard, we need to have a fixed gap between grid items.
maxSpace
will unlock this possibility without having to resort to artificially reduce the Virtualizer width with additional pre-computations