-
Notifications
You must be signed in to change notification settings - Fork 13.4k
fix(select): do not collapse to width: 0 when placed in flex container #28631
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
Conversation
a8d240e
to
a5a007e
Compare
390d1b2
to
aee7bc3
Compare
99561a4
to
ab11766
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.
Confirmed that this works with the extra ion-item
👍 Once this merges, I'll get a separate PR up to remove the min-width
patch from the select slot playground.
.../item/test/disabled/item.e2e.ts-snapshots/item-disabled-diff-ios-ltr-Mobile-Chrome-linux.png
Show resolved
Hide resolved
...est/slotted-inputs/item.e2e.ts-snapshots/item-slotted-inputs-ios-ltr-Mobile-Chrome-linux.png
Show resolved
Hide resolved
...s/item/test/legacy/fill/item.e2e.ts-snapshots/item-fill-diff-ios-ltr-Mobile-Chrome-linux.png
Show resolved
Hide resolved
...tem/test/highlight/item.e2e.ts-snapshots/item-highlight-diff-ios-ltr-Mobile-Chrome-linux.png
Show resolved
Hide resolved
<ion-select-option value="apple">Apple</ion-select-option> | ||
</ion-select> | ||
</ion-item> | ||
<ion-list> |
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.
Brandy and I discussed that it would be good to account for ion-list
here too. Amanda had found an edge case where Brandy's previous fix did not work, so we wanted to test for that 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.
Talked through this with Liam - the change makes sense & we can always remove the CSS variable later since it is internal. Great job!
// allowing the item to grow to fill the flex container. | ||
// If the item is inside of a block container this | ||
// property will be ignored. | ||
flex: 1; |
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 is the old fix that brandy had that is no longer needed.
Note: The screenshot diffs in 07ab2fe are correct. I wrapped the item in a list as per #28631 (comment). As a result, items in a flex container no longer grow by default. This test is verifying that the select widths don't collapse to 0px. This behavior aligns with what's in main (v7.5.7 as of writing): https://codepen.io/liamdebeasi/pen/bGzOxjy |
Issue number: Internal
What is the current behavior?
We currently apply a workaround to
ion-select
so it can wrap correctly inside ofion-item
:ionic-framework/core/src/components/select/select.scss
Lines 99 to 103 in 357b8b2
However, this causes issues when a parent element has
display: flex
because theion-select
width becomes 0.What is the new behavior?
ion-select
(and other elements in the default slot) to either truncate or wrap within its own container and then have the entire container (i.e. the entireion-select
) wrap to the next line once the container is too small.To achieve this, I needed to set a min-width on
.item-inner
to define the point at which the element should wrap to the next line. I also changed the flex basis fromauto
to0
which means the initial main size of the flex item will be 0px. In reality, this will be--inner-min-width
since we also setmin-width: var(--inner-min-width)
. I used0
for simplicity but I can change this to use the CSS variable if that's more clear. Since we also setflex-grow: 1
we indicate that the element can grow from that basis (but it cannot shrink).I chose
--inner-min-width: 4rem
to minimize the number of diffs. We can certainly change this, but it may cause some diffs as certain elements will start wrapping sooner. I also chose to userem
because having a fixed min-width means that fewer characters are going to fit in the same space as text scales.I made this a CSS variable but left it undocumented. If developers need a way of changing this
min-width
they can request it and we can easily expose this variable. However, I think4rem
is small enough that this should be sufficient.Does this introduce a breaking change?
Other information
The visual diffs here are correct. The table below shows the screenshot group and an explanation for why the changes are correct.
disabled
highlight
--inner-min-width
.legacy/fill
--inner-min-width
.slotted-inputs
--inner-min-width
.slotted-inputs
note: I'd argue many of these examples are not best practices. For example, adding a range in the start slot and the end slot is a bit unusual. I'm not aware of any native apps that implement this pattern.popover note: I removed the
ion-item
from thepopover/test/async
test. There was a diff because the min-width increased, but IMO that component should not be used in the popover test since we want to test the popover, not the item.Demo:
feature-7.6
branch
before-item.mov
after-item.mov
(In this demo I updated the
ion-select
to wrap within its own container first instead of truncate. We may want to consider doing this by default, but I think this is out of scope for this task)