-
Notifications
You must be signed in to change notification settings - Fork 124
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
Grid layout #570
Grid layout #570
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 example works well. The UX pattern shown is a bit weird, but it achieves the aims.
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. The way I did it in the example isn't intended for typical use. Probably just spreadsheets and things like calc, where I had the top flex span all four cells.
Co-authored-by: Daniel McNab <[email protected]>
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.
The recent commit addresses most of the issues other than:
- Documentation
- Optimization of layout calls
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. The way I did it in the example isn't intended for typical use. Probably just spreadsheets and things like calc, where I had the top flex span all four cells.
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.
Overall it's a really well-written PR. I especially appreciate the tests.
There are a few minor things I'd want to change, but the only thing that's truly blocking is the commented-out code. Aside from that, this is good to merge.
let mut grid = grid.downcast::<Grid>(); | ||
grid.add_child(button::Button::new("A"), GridParams::new(0, 0, 1, 1)); | ||
}); | ||
assert_render_snapshot!(harness, "initial_2x2"); // Should be back to the original state |
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.
Huh. You reuse the same snapshot multiple times. That's pretty clever.
Co-authored-by: Daniel McNab <[email protected]>
One more thing I'd like is a test for negative spacing. Otherwise, no comment, it's a very clean PR. |
Awesome! |
This PR adds a basic grid layout to Masonry and Xilem.
The way this layout works is it has a fixed grid based on the initial size passed in, and grid items are placed based on the position requested. Grid items are allowed to span more than one cell, if requested.
There are potential improvements that could be done, like the use of intrinsic sizing for varied column width based on content size. Though that could be done in the future taffy layout if we want to keep this one simple.
This PR is still a draft because of one remaining concern. I was not able to successfully optimize with conditional calls to child widgets for layout. It led to crashes about the paint rects not being within the widget's paint rect.Error in 'Grid' #16: paint_rect Rect { x0: 0.0, y0: 0.0, x1: 800.0, y1: 610.0 } doesn't contain paint_rect Rect { x0: 400.5, y0: 0.0, x1: 800.5, y1: 150.0 } of child widget 'Button' #5
. My failed attempt at fixing it is commented out.Since I am rusty on View Sequences, a lot of that code is based on the Flex implementation. Let me know if I did anything incorrectly or if any of it is unnecessary, or if anything is missing.