Skip to content

Commit

Permalink
fix(select): do not collapse to width: 0 when placed in flex container (
Browse files Browse the repository at this point in the history
#28631)

Issue number: Internal

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

We currently apply a workaround to `ion-select` so it can wrap correctly
inside of `ion-item`:
https://github.com/ionic-team/ionic-framework/blob/357b8b2beb29b95d53ef043af349067be1d32658/core/src/components/select/select.scss#L99-L103

However, this causes issues when a parent element has `display: flex`
because the `ion-select` width becomes 0.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- In order to get the desired behavior, we need the `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 entire
`ion-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 from `auto` to `0` which means the initial main
size of the flex item will be 0px. In reality, this will be
`--inner-min-width` since we also set `min-width:
var(--inner-min-width)`. I used `0` for simplicity but I can change this
to use the CSS variable if that's more clear. Since we also set
`flex-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 use `rem` 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 think `4rem` is small enough that this
should be sufficient.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

The visual diffs here are correct. The table below shows the screenshot
group and an explanation for why the changes are correct.


| Path | Example | Details |
| - | - | - |
| `disabled` |
[Link](https://github.com/ionic-team/ionic-framework/pull/28631/files#diff-d529716f95f7a7aa82c88588104220775b728af67077f48cd47a8afa04423143)
| The searchbar is able to shrink slightly to fit on the same line as
the checkbox at the bottom. |
| `highlight` |
[Link](https://github.com/ionic-team/ionic-framework/pull/28631/files#diff-0b64f24c91393923701d1ced4e330a1c6b926d72ee461b8ab1e135e708be3457)
| We're changing how small the main content can get, so the input is
only wrapping once it gets to `--inner-min-width`. |
| `legacy/fill` |
[Link](https://github.com/ionic-team/ionic-framework/pull/28631/files#diff-2ef8dbfa5e69e2b96c3e1ed29ab962f08cf5ba2aaf2af773e40bd143e38a4bef)
| We're changing how small the main content can get, so the input is
only wrapping once it gets to `--inner-min-width`. |
| `slotted-inputs` |
[Link](https://github.com/ionic-team/ionic-framework/pull/28631/files#diff-2f173c7303969d6a6c58f30a618cebc3caf918d3761fc83df5642fd48dfabd7b)
| We're changing how small the main content can get, so the range is
only wrapping once it gets to `--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 the `popover/test/async`
test](331fcb8).
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` |
| - | - |
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/693d4947-fa33-460d-bc7f-7b96b6338032"></video>
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/df35ca73-87aa-4e76-9bb7-99f0f2810640"></video>
|

(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)

---------

Co-authored-by: ionitron <[email protected]>
Co-authored-by: Brandy Carney <[email protected]>
  • Loading branch information
3 people authored Dec 5, 2023
1 parent 8c235fd commit e71e7a0
Show file tree
Hide file tree
Showing 46 changed files with 30 additions and 24 deletions.
29 changes: 19 additions & 10 deletions core/src/components/item/item.scss
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,15 @@
* @prop --highlight-color-valid: The color of the highlight on the item when valid. Only applies to inputs and textareas using the legacy form syntax. DEPRECATED: Highlights can be styled on `ion-input` or `ion-textarea` when using the modern form syntax.
* @prop --highlight-color-invalid: The color of the highlight on the item when invalid. Only applies to inputs and textareas using the legacy form syntax. DEPRECATED: Highlights can be styled on `ion-input` or `ion-textarea` when using the modern form syntax.
*/

/**
* We change the minimum width as the
* font size changes. Using a fixed minimum
* width means that fewer and fewer characters
* can be displayed in the same space as the
* text grows.
*/
--inner-min-width: 4rem;
--border-radius: 0px;
--border-width: 0px;
--border-style: solid;
Expand Down Expand Up @@ -80,15 +89,6 @@

position: relative;

// When an item containing a select is inside of a
// flex container the item will collapse to 0px
// width due to the select setting the width to 0px.
// By setting the flex property here, we are
// 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;

align-items: center;
justify-content: space-between;

Expand Down Expand Up @@ -310,7 +310,7 @@ button, a {
// This flex property is required in order to make
// the elements wrap when there is a slotted start
// element and a label
flex: 1 0 auto;
flex: 1 0 0;

flex-direction: inherit;

Expand All @@ -322,6 +322,15 @@ button, a {
align-items: inherit;
align-self: stretch;

/**
* The min-width defines when the
* content in the default slot should
* stop wrapping/truncating within its own
* container. At this point the entire
* container will wrap to the next line.
*/
min-width: var(--inner-min-width);

// Max width must be set to 100%, otherwise the
// elements will overflow this container instead
// of wrapping
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
4 changes: 1 addition & 3 deletions core/src/components/popover/test/async/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,7 @@
popover.addEventListener('ionMount', () => {
popover.innerHTML = `
<div style="padding: 10px;">
<ion-list>
<ion-item>Item 1</ion-item>
</ion-list>
Popover Content
</div>
`;
});
Expand Down
9 changes: 3 additions & 6 deletions core/src/components/select/select.scss
Original file line number Diff line number Diff line change
Expand Up @@ -92,14 +92,11 @@

// TODO FW-3194 - Remove the :not(.legacy-select) piece
//
// The flex and width properties are required here
// in order to allow the select to shrink inside of an item
// otherwise it always wraps to the next line even
// when it can shrink
// The flex property is required here in order to allow
// the select to shrink inside of an item otherwise it
// always wraps to the next line even when it can shrink
:host(.in-item:not(.legacy-select)) {
flex: 1 1 0;

width: 0;
}

// TODO FW-3194 - Remove this
Expand Down
12 changes: 7 additions & 5 deletions core/src/components/select/test/item/select.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,13 @@ configs({ directions: ['ltr'] }).forEach(({ title, screenshot, config }) => {
await page.setContent(
`
<div id="container" style="display: flex">
<ion-item>
<ion-select label="Fruit" value="apple">
<ion-select-option value="apple">Apple</ion-select-option>
</ion-select>
</ion-item>
<ion-list>
<ion-item>
<ion-select label="Fruit" value="apple">
<ion-select-option value="apple">Apple</ion-select-option>
</ion-select>
</ion-item>
<ion-list>
</div>
`,
config
Expand Down

0 comments on commit e71e7a0

Please sign in to comment.