-
Notifications
You must be signed in to change notification settings - Fork 112
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
Fix sizing of flexbox nodes under a min-content constraint #291
Fix sizing of flexbox nodes under a min-content constraint #291
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.
Needs a release notes entry :) I suspect you're right about the need to expand the cache anyways.
I also think we should try to improve the clarity of the cache_slot code.
src/compute/mod.rs
Outdated
// Compute cache slot | ||
let has_known_width = known_dimensions.width.is_some(); | ||
let has_known_height = known_dimensions.height.is_some(); | ||
let cache_slot = if has_known_width && has_known_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.
The logic here is hard to follow, and seems likely to result in surprising bugs if altered unsuspectingly. Is there a clearer or more robust approach we can use? It feels like a match statement may help as a stopgap change.
Failing that, more comments.
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 think the logic itself is good (for the time being at least), but I agree that it could do with some explaining / documenting. I remember trying to understand the caching implementation when I first started working on Taffy, and it made no sense to me at all.
I always forget those! |
@alice-i-cecile Release notes added, and I've moved the cache slot logic to a function and added documentation to the function. Let me know if it makes any more sense now. |
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.
Much better! May still want to refactor the caching code later, but this is a great base for that.
Objective
Nesting a flexbox row inside a grid column with a min-content width was giving incorrect results.
Context
This was discovered in testing Grid/Flexbox interaction, although I believe it was a pre-existing bug in the Flexbox implementation that was exposed by the Grid implementation implementing
min-content
.Changes made