-
Notifications
You must be signed in to change notification settings - Fork 152
Add Stack widget #1292
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
base: main
Are you sure you want to change the base?
Add Stack widget #1292
Conversation
b8d0f29
to
be61396
Compare
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.
Does this fix #607?
It's not clear to me why this is only used in a single example - do so many of our examples and other code make use of "advanced" flex features?
If so, it's not all that clear what the purpose of this type actually is.
(That isn't entirely clear to me anyway, to be honest).
Code looks good other than my comments
/// All the children of this widget will be laid out with the same constraints, | ||
/// then placed one after the other depending on the set alignment. |
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.
What are these same constraints? It's really helpful for a mental model if this is explained, even if we expect it to change later.
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.
It's parent_box_constraints.loosen()
, which is essentially "same maximum sized at the parent, no minimum size".
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.
harness.edit_root_widget(|mut stack| { | ||
Stack::set_cross_axis_alignment(&mut stack, CrossAxisAlignment::Fill); | ||
}); | ||
assert_render_snapshot!(harness, "stack_row_cross_axis_fill"); |
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 don't think that this test meaningfully shows a fill (because e.g. the label is always left aligned).
Maybe (at least for the vertical ones) giving the labels internally centre alignment would work? OTOH, if we ever got a safety rail for that case, we'd run into issues...
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 I should give them a border.
So my ideal plan for this would be to track down flex uses in examples which don't set After that, I'd want to straight up remove the
Weeell, I don't know. I'd argue not. This is still more complex than an hypothetical "simplest container you can write" widget: it has axis-independent code, which makes the whole code really hard to read until you internalize what "minor" and "major" mean (every time I spend more than six months without looking at the flex code, I have a learning period the next time I see it where I have to get used to those idioms again). It also deals alignment and spreading extra space, whereas a "simplest possible container" widget would just align everything top-left. |
How this will work is a little bit unclear to me still. Since the Stack widget supports distributing its spacing, it seems like it also sometimes fills the main axis? I'm probably misunderstanding something fundamental here. |
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 this discussion blocks this PR landing.
I'd really like to see the test items have a border here, and I'm approving conditional on that.
Right, so, the Stack widget distributes its spacing when it needs to fill the minimum constraint. Ideally, I'd like to remove min constraints for Masonry entirely, and remove extra space code from Stack. Though we could also remove the MainAxisAlignment param, assume that alignment is always at the start, and remove most of that code.
I know I've asked you to expedite the process before, but I'm actually okay with delaying this PR a little. It's not critical for anything, and those questions are worth exploring. |
Add Stack view.
Include test screenshots.
Stack is a non-flexible container widget, for cases where a Flex is not needed.