Skip to content
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

Clarify interaction between collapsing and multi-line containers #36791

Merged
merged 11 commits into from
Dec 18, 2024

Conversation

shardulc
Copy link
Contributor

Description

I'm not entirely sure what the preëxisting example intended to demonstrate, so I changed it and its description to illustrate the spec quoted right above it. Let me know if there's some aspect of the spec that this example doesn't cover.

Motivation

As it stands, I find the second example a bit confusing.

Additional details

Relevant section of the CSS flexbox specification in the Editors’ Draft and the published CR.

@shardulc shardulc requested a review from a team as a code owner November 14, 2024 18:13
@shardulc shardulc requested review from estelle and removed request for a team November 14, 2024 18:13
@github-actions github-actions bot added Content:CSS Cascading Style Sheets docs size/s [PR only] 6-50 LoC changed labels Nov 14, 2024
Copy link
Contributor

github-actions bot commented Nov 26, 2024

Preview URLs

(comment last updated: 2024-12-18 20:12:38)

…_of_flex_items/index.md

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Member

@estelle estelle left a comment

Choose a reason for hiding this comment

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

Hi @shardulc,

Apologies for not getting back to this sooner. When you are ready for a re-review, if you click on the icon (that looks like a refresh page icon now, but may look like a document icon in red if changes are required) next to the reviewer's name, that lets the review know it is ready for a re-review.

I made several suggestions to hopefully make is super clear what is going on. I'm hoping it is so clear that even if someone doesn't have Firefox, it's described well enough that they may understand what is happening. If that's the case, we got it right.

estelle and others added 5 commits December 18, 2024 09:17
…_of_flex_items/index.md

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot added size/m [PR only] 51-500 LoC changed and removed size/s [PR only] 6-50 LoC changed labels Dec 18, 2024
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
estelle and others added 2 commits December 18, 2024 12:04
…_of_flex_items/index.md

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Copy link
Member

@estelle estelle left a comment

Choose a reason for hiding this comment

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

I added toggles so people can see the effect without needing to open play.

Thank you.

BTW, congratulations on your first merged MDN content PR. Welcome to the team!

@estelle estelle merged commit c31ea3d into mdn:main Dec 18, 2024
8 checks passed
@shardulc
Copy link
Contributor Author

Thanks for working on this too! Sorry I didn't respond to your review in time, it was on my holiday TODO list 😅

Copy link
Contributor Author

@shardulc shardulc left a comment

Choose a reason for hiding this comment

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

Sorry to be a bit nitpicky again but just noticed two small details that we might as well address since we're already working on this

@@ -261,16 +261,24 @@ The flexbox specification details what should happen if a flex item is collapsed

This behavior is useful if you want to target flex items using JavaScript to show and hide content for example. The example in the specification demonstrates one such pattern.

In the following live example, I have a non-wrapped flex container. The third item has more content than the others yet is set to `visibility: collapse`; therefore, the flex container is retaining a _strut_ of the height required to display this item. If you remove `visibility: collapse` from the CSS or change the value to `visible`, you will see the item appear, and the space is redistributed between non-collapsed items; the height of the flex container should not change.
In the following live example, the non-wrapping flex container contains a row with three flex items that are set to flex to equal sizes. The third item has multiple lines of content, growing the container. The default for `align-items` is `normal`; for flex items, `normal` behaves as `stretch`, so all the items stretch by default, filling the container's cross-size height.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

"the non-wrapping flex container contains a row with three flex items": I wonder if it could be confusing to say it contains a row, when really it is just a single row, being a single-line flex container.


This means that items might end up on a different line to the one they started on. In the case of an item being shown and hidden it could well cause the items to end up in a different row.
The following example shows this behavior. The third flex item is collapsed, so it occupies zero space along the main axis (the inline-size is `0`). When collapsed, its strut is on the first row after the fourth item, with the first row being tall enough to fit the three lines of text that the third item would have had. Then, if you uncollapse the item (e.g. by removing the `collapse` class), there is no longer enough horizontal space for the fifth item on the first row, and it moves to the second. This causes the second row to grow to fit the two lines of text of its new member, and the last flex item is pushed onto a new row. With a taller second line and a new third line, the flex container is much taller than it was before.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The description does not match the demo (at least for me, on Firefox), but it does if the width of the container is increased from 500px to 520px.

allan-bonadio pushed a commit to allan-bonadio/content that referenced this pull request Dec 25, 2024
…#36791)

* Clarify interaction between collapsing and multi-line containers

* Update files/en-us/web/css/css_flexible_box_layout/mastering_wrapping_of_flex_items/index.md

* Update files/en-us/web/css/css_flexible_box_layout/mastering_wrapping_of_flex_items/index.md

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Apply suggestions from code review

* Update files/en-us/web/css/css_flexible_box_layout/mastering_wrapping_of_flex_items/index.md

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* add toggle for visibility collapse

* toggle button for example

* Apply suggestions from code review

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Apply suggestions from code review

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Update files/en-us/web/css/css_flexible_box_layout/mastering_wrapping_of_flex_items/index.md

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* change class name to collapse

---------

Co-authored-by: Estelle Weyl <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:CSS Cascading Style Sheets docs size/m [PR only] 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants