Skip to content

Track sizing support in Grid widget and view#1794

Open
RagibHasin wants to merge 11 commits into
linebender:mainfrom
RagibHasin:greedy
Open

Track sizing support in Grid widget and view#1794
RagibHasin wants to merge 11 commits into
linebender:mainfrom
RagibHasin:greedy

Conversation

@RagibHasin

Copy link
Copy Markdown
Contributor

This completely reworks how the Grid widget lays its children out.

The new implementation closely follows CSS grid specification with one intentional deviation, it does not support minmax track size. It has been omitted from track sizing MVP and because I don't find it very useful.

@RagibHasin
RagibHasin force-pushed the greedy branch 2 times, most recently from 7fd925e to 62cc580 Compare May 13, 2026 16:36
@RagibHasin
RagibHasin marked this pull request as ready for review May 13, 2026 16:36
@RagibHasin
RagibHasin requested review from PoignardAzur and xStrom and removed request for xStrom May 13, 2026 16:38
@RagibHasin
RagibHasin force-pushed the greedy branch 2 times, most recently from f42ed69 to 3bc58b7 Compare May 13, 2026 17:40
@PoignardAzur

Copy link
Copy Markdown
Contributor

Thanks for the work! Not sure I'll have time to review it in the coming weeks, but I definitely appreciate changes that get us closer to CSS Grid behavior.

At first glance, my impression is that this PR makes a lot of drive-by changes to the API, some of which I don't agree with. For instance, I don't see a reason to remove GridExt::grid_pos, and I think replacing GridParams::new with tuples makes the code harder to read.

Anyway, I'll do a more in-depth review soon. This is work we really want to get merged!

@RagibHasin

Copy link
Copy Markdown
Contributor Author

I don't see a reason to remove GridExt::grid_pos

I removed that and unified its functionality into GridExt::grid to be similar to FlexExt::flex. Also, GridParams implements From<(u16, u16)> for position items, making grid_pos more verbose.

I think replacing GridParams::new with tuples makes the code harder to read.

I would replace the tuples with GridParams builder methods if requested. GridParams::new() is now a default constructor as it should be, IMO.

@RagibHasin
RagibHasin force-pushed the greedy branch 2 times, most recently from 1e27c91 to f672300 Compare May 20, 2026 03:11
@PoignardAzur

Copy link
Copy Markdown
Contributor

(I'm back from RustWeek, will probably review this tomorrow!)

@PoignardAzur PoignardAzur left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some general thoughts:

  • I maintain that the impl From<...> with tuples are a bad idea. They're way more error-prone than the builder methods, they're less IDE-friendly (IDEs can show you function argument names, but not for tuples), they're less explicit for someone reading the code, etc. Unless there's a very strong argument to have them (other than "it makes code shorter") I think I'll block the feature until they're removed, sorry.
  • I think you should keep the Grid::with_dimensions builder, since I expect it to be useful for a lot of examples. You could rename it and/or document that it creates tracks with GridTrackSize::FRACTION, or even pass the track size as a parameter.
  • Overall, I think it would be helpful to split this PR in two, one that deals with the renamed constructors, then one that deals with the logic changes. I won't block on it though.

Sorry to wait so long just to add more negative feedback. I still need to review the grid code itself, but I do think the ergonomics part is important on its own.

Comment thread masonry/examples/grid_masonry.rs Outdated
Comment thread xilem_masonry/src/view/grid.rs Outdated
@RagibHasin

Copy link
Copy Markdown
Contributor Author
  • I maintain that the impl From<...> with tuples are a bad idea. They're way more error-prone than the builder methods, they're less IDE-friendly (IDEs can show you function argument names, but not for tuples), they're less explicit for someone reading the code, etc. Unless there's a very strong argument to have them (other than "it makes code shorter") I think I'll block the feature until they're removed, sorry.

My rationale for this was similar impls for FlexParams. I agree that the lack of parameter name hints would hinder unfamiliar users. Also, the From impls feel very much like a DSL.

But I don't like the impending verbosity of the builder patterns, too. 🤷🏽

I think if we revert to GridExt::grid_pos with (x: impl Into<Option<u16>>, ..) as parameters, we can avoid most of the verbosity. Would the impl Into<Option<u16>> tank compile times heavily?

  • I think you should keep the Grid::with_dimensions builder, since I expect it to be useful for a lot of examples. You could rename it and/or document that it creates tracks with GridTrackSize::FRACTION, or even pass the track size as a parameter.

I have no opinion in this regard, so I would re-establish Grid::with_dimensions.

  • Overall, I think it would be helpful to split this PR in two, one that deals with the renamed constructors, then one that deals with the logic changes. I won't block on it though.

That would be painful. I would like to skip splitting it.

@PoignardAzur

Copy link
Copy Markdown
Contributor

I think if we revert to GridExt::grid_pos with (x: impl Into<Option>, ..) as parameters, we can avoid most of the verbosity. Would the impl Into<Option> tank compile times heavily?

I think the compile times would be fine, but also I think that's over-engineering. In most cases I expect users to either specify both coordinates or nothing. grid_pos(u16, u16) should be fine.

That would be painful. I would like to skip splitting it.

Fair enough.

@RagibHasin
RagibHasin requested a review from PoignardAzur July 5, 2026 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants