-
Notifications
You must be signed in to change notification settings - Fork 114
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
Fixes for aspect ratio and min/max sizes #317
Conversation
015934e
to
b28deaa
Compare
I'm not sure if this covers everything (in fact I have 4 disabled tests), but aspect ratio is working a lot better with this PR than it was before (including the use case in #313), and I'm intending to move on to other tasks now and return to aspect ratio again later if required. |
@nicoburns hey! // aspect_ratio_leaf_inset_width
<div id="test-root" style="display: flex; width: 1280px; height: 720px;">
<div style="position: absolute; top: 5%; left: 5%; width: 20%; aspect-ratio: 3;"></div>
</div> |
@adjabaev Well spotted! I've fixed that case, but I think I'll need to test more cases using absolute positioning. |
7ff739f
to
fd02d7f
Compare
fd02d7f
to
86509aa
Compare
It might be a mistake but both tests you commited (fixtures) have identical content On a side note, I'll keep trying getting crazy with aspect ratio and keep you updated if I end up finding something behaving weirdly |
Ah yes, that'll be because the "rename" and "duplicate" buttons are right next to each other in my editor. I wanted to rename it "absolute" because it turns out that percentages have nothing to do with it. |
… to text node content sizing
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.
Some nitpicks and questions :)
It now: - Applies margins correctly - Applies min/max correctly - Applies aspect ratio correctly
…ive to the grid container
022a092
to
fad8919
Compare
Co-authored-by: Andreas Weibye <13300393+Weibye@users.noreply.github.com>
…d item in container with border
@alice-i-cecile This is now ready for another round of review / merging when ready. |
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.
Looks good, I haven't been able to spot any issues but as usual I'm relying on your expertise and the tests to vouch for the correctness of the implementation.
match (align_self, constants.is_wrap_reverse) { | ||
// Stretch alignment does not apply to absolutely positioned items | ||
// See "Example 3" at https://www.w3.org/TR/css-flexbox-1/#abspos-items | ||
// Note: Stretch should be FlexStart not Start when we support both | ||
(AlignSelf::Baseline | AlignSelf::Stretch | AlignSelf::Start, false) | (AlignSelf::End, true) => { | ||
constants.padding_border.cross_start(constants.dir) + resolved_margin.cross_start(constants.dir) | ||
} | ||
AlignSelf::End => { | ||
if constants.is_wrap_reverse { | ||
constants.padding_border.cross_start(constants.dir) | ||
} else { | ||
free_cross_space - constants.padding_border.cross_end(constants.dir) | ||
} | ||
(AlignSelf::Baseline | AlignSelf::Stretch | AlignSelf::Start, true) | (AlignSelf::End, false) => { | ||
constants.container_size.cross(constants.dir) | ||
- constants.padding_border.cross_end(constants.dir) | ||
- final_size.cross(constants.dir) | ||
- resolved_margin.cross_end(constants.dir) | ||
} | ||
AlignSelf::Center => free_cross_space / 2.0, | ||
AlignSelf::Baseline => free_cross_space / 2.0, // Treat as center for now until we have baseline support | ||
AlignSelf::Stretch => { | ||
if constants.is_wrap_reverse { | ||
free_cross_space - constants.padding_border.cross_end(constants.dir) | ||
} else { | ||
constants.padding_border.cross_start(constants.dir) | ||
} | ||
(AlignSelf::Center, _) => { | ||
(constants.container_size.cross(constants.dir) | ||
+ constants.padding_border.cross_start(constants.dir) | ||
- constants.padding_border.cross_end(constants.dir) | ||
- final_size.cross(constants.dir) | ||
+ resolved_margin.cross_start(constants.dir) | ||
- resolved_margin.cross_end(constants.dir)) | ||
/ 2.0 |
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 particularly like this way of structuring this code!
@@ -42,6 +43,19 @@ impl<T: Default> Default for Rect<T> { | |||
} | |||
} | |||
|
|||
impl<U, T: Add<U>> Add<Rect<U>> for Rect<T> { |
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 deserves a note in the release notes; it's surprisingly useful.
Objective
Fix #313 (and more generally fix support for aspect-ratio).
This PR now also fixes #167
Subtasks
Context
Taffy has an
aspect_ratio
style property and claims to support CSSaspect-ratio
. But actual support is patchy, and it is not covered by gentests.