Skip to content

fix(item): elements should wrap and label should not collapse when text size increases - #28146

Merged
brandyscarney merged 80 commits into
feature-7.6from
FW-4905
Oct 17, 2023
Merged

fix(item): elements should wrap and label should not collapse when text size increases#28146
brandyscarney merged 80 commits into
feature-7.6from
FW-4905

Conversation

@brandyscarney

@brandyscarney brandyscarney commented Sep 7, 2023

Copy link
Copy Markdown
Member

This will resolve the following internal tickets:

  • FW-4905: bug: label should not collapse in an item or item divider with a note
  • FW-5092: bug: label should wrap instead of truncating inside of an item

What is the current behavior?

There are a few problems with how labels inside of items behave. Labels inside of items do not wrap their text, instead they truncate with an ellipsis. The label will continue to show an ellipsis while shrinking if there is wide content next to it, until eventually it completely disappears. This is especially problematic for dynamic type, when the font size can be increased up to 310% on ios. Below are some screenshots of how Ionic behaves when increasing the font size with an ion-label and ion-note in an ion-item. You'll notice that the ion-label (Wi-Fi) completely disappears at 310%.

100% 145% 310%
ios ionic 100% ios ionic 145% ios ionic 310%

Here is how the same item looks on native iOS at 310%:

ios native 310%

What is the new behavior?

A lot of changes were made in order to support wrapping components inside of an item. Since a lot of the code in ion-label and ion-item is used throughout multiple components, several changes were wrapped into this PR.

  1. Item scss was modified to allow wrapping of its containers. Comments have been added to explain the new additions.
  2. Label scss has been updated to allow wrapping (removing the white-space: nowrap) and allow it to grow.
  3. A legacy class was added to item (.item-legacy) by all of the components that have legacy as a property. This class was targeted in the CSS to undo the wrapping changes for legacy items, otherwise it breaks a lot of tests: 6ec8bf4
    a) Tests with legacy inputs were updated to include the legacy attribute. Without it the UI will break for several examples: a8a34ce
  4. The item/test/text test was removed. It was almost exactly a duplicate of item/test/basic: eb79794
  5. The Item inside of Datetime was styled to set the width to auto, undoing where I set the width to min-content elsewhere, in order to prevent it from wrapping all of the time. I also added a comment linking to a JIRA ticket to remove the Item because of the issues it causes by being used: 88106d8
  6. Added some CSS to allow the select to shrink when inside of an item, otherwise it always wraps to the next line even when it can shrink and fit: 6804aea
  7. Adds a test to a11y for Note to ensure that the Label next to a Note doesn't disappear: 1f23ffc
  8. Some tests were updated to remove the ion-text-wrap and/or add ion-text-nowrap to prevent screenshot diffs. For example
  9. Passes the skeleton-text class up to item (as item-skeleton-text) in order to add overflow: hidden to the label for skeleton text wrappers: 077fb08 - this also adds overflow: hidden for legacy labels to keep the ellipsis but the modern items do not need it

Does this introduce a breaking change?

  • Yes
  • No

Other information

Screenshots for this branch are separated into the following PR: #28278

liamdebeasi and others added 30 commits July 6, 2023 15:17
Issue number: N/A

---------

## What is the current behavior?
The datetime button does not support dynamic type.

## What is the new behavior?
Adds support for dynamic type by converting `font-size` to use `rem`
units and adding an ellipsis to text when the size is too large.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No


## Other information

Dynamic type can be enabled with the following CSS:

```css
html {
  --ion-dynamic-type: var(--ion-default-dynamic-font);
}
```
Adds support for dynamic type by converting `font-size` to use `rem` units and setting a `max-size` which matches native iOS.
- Adds support for dynamic type by converting `font-size` to use `rem` units.
- Updates the button width/height for icon only buttons in `md` to use `rem` units to scale the icon larger
- Adds test for scaling the font which checks default buttons, clear buttons (because they use a different `font-size`), buttons with an icon, and a button containing only an icon
Issue number: N/A

---------

<!-- 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. -->
The chip component does not support dynamic type.

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->
The chip component supports dynamic type, has a dynamic height, has min
and max font sizes in `ios` mode, and has a screenshot test.

### md mode

Small md mode:
<img width="1368" alt="md-small"
src="https://github.com/ionic-team/ionic-framework/assets/14926794/4466c1b6-6d01-4ec8-8de9-0eb71fa45223">


Medium md mode:
<img width="1371" alt="md-medium"
src="https://github.com/ionic-team/ionic-framework/assets/14926794/c51c324e-a3b7-486b-9f68-84b0e4146570">


Large md mode:
<img width="1369" alt="md-large"
src="https://github.com/ionic-team/ionic-framework/assets/14926794/166e1229-96b7-4270-9ef2-4c708776691e">


 
### ios mode
Small ios mode:
<img width="1371" alt="ios-small"
src="https://github.com/ionic-team/ionic-framework/assets/14926794/de240058-5fe5-4387-8481-d695950853db">


Medium ios mode:
<img width="1371" alt="ios-medium"
src="https://github.com/ionic-team/ionic-framework/assets/14926794/c63db4be-e813-4917-a525-02ec456849ea">


Large ios mode:
<img width="1370" alt="ios-large"
src="https://github.com/ionic-team/ionic-framework/assets/14926794/3479f619-82c8-4645-85f7-2725ea43de00">



## 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. -->

This also fixes an existing bug that the contents of a chip could get
smooshed if the chip doesn't have enough horizontal space for its
contents.

### Before
<img width="253" alt="rStll"
src="https://github.com/ionic-team/ionic-framework/assets/14926794/9a8f4c30-ceef-4fe1-a600-38b2f01c4e68">


### After
<img width="253" alt="Screenshot 2023-08-02 at 5 00 39 PM"
src="https://github.com/ionic-team/ionic-framework/assets/14926794/e82e17c3-91e3-4089-a57b-16c7d7b386fb">

---------

Co-authored-by: ionitron <hi@ionicframework.com>
Co-authored-by: Liam DeBeasi <liamdebeasi@users.noreply.github.com>
Adds support for dynamic type by converting `font-size` to use `rem` units and setting a max `font-size` which matches native iOS.
Adds support for dynamic type by converting `font-size` to use `rem` units and setting a min `font-size` which matches native iOS.
Adds support for dynamic type by converting `font-size` to use `rem` units.
Adds support for dynamic type by converting `font-size` to use `rem` units.
Adds support for dynamic type by converting `font-size` to use `rem` units and adds a min and max `font-size` for `ios`.
Issue number: N/A

---------

<!-- 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. -->

The dynamic type feature branch screenshots are out of sync with the
updated state from changes on `main`.

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

- Regenerates update screenshot diffs for the dynamic type feature
branch with the latest changes from `main`

## 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. -->

Co-authored-by: ionitron <hi@ionicframework.com>
Adds support for dynamic type by converting `font-size` to use `rem` units. Upgrades the `sass` version to `1.33.0` to support this change.
Adds support for dynamic type by converting `font-size` to use `rem` units and adds a min and max `font-size` for `ios`.
Issue number: N/A

---------

<!-- 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. -->
ion-textarea does not support dynamic type

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

- ion-textarea does support dynamic type

## 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. -->

Mostly, dynamic type for textarea was already supported. Only the
bottom-content had a static size. All other font sizes were all already
relative.

Note: Textareas do not currently look correct when they are in an item,
because #27989 has not yet merged. Once that PR is merged, the font
sizes within textareas will be relative to the item sizes, and both will
scale appropriately. If preferred, I can base this PR off branch FW-4432
instead.

---------

Co-authored-by: ionitron <hi@ionicframework.com>
Co-authored-by: ionitron <hi@ionicframework.com>
Co-authored-by: Liam DeBeasi <liamdebeasi@users.noreply.github.com>
@brandyscarney
brandyscarney changed the base branch from feature-7.5 to feature-7.6 October 12, 2023 16:18
Comment thread core/src/components/skeleton-text/skeleton-text.tsx

@liamdebeasi liamdebeasi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One comment, but good to go once that's addressed.

@brandyscarney
brandyscarney merged commit 6438e3e into feature-7.6 Oct 17, 2023
@brandyscarney
brandyscarney deleted the FW-4905 branch October 17, 2023 18:08
@tgangso

tgangso commented Dec 7, 2023

Copy link
Copy Markdown

Not sure I agree with this not being a breaking change, had to go and put in ion-text-nowrap in a lot of places in my app. Should be mentioned in the notes.

@liamdebeasi

Copy link
Copy Markdown
Contributor

I've updated the release notes, but I recommend allowing text to wrap by default. The previous behavior was broken from an accessibility perspective, so forcing text to not wrap can degrade the experience for many users. We don't consider this a breaking change because the previous experience was broken for many users.

github-merge-queue Bot pushed a commit that referenced this pull request Dec 7, 2023
We got some feedback in
#28146 (comment)
that it would be helpful to note the new item wrapping behavior in v7.6.
I've updated the Github release, and this PR updates the Changelog file.
@tgangso

tgangso commented Dec 7, 2023

Copy link
Copy Markdown

I've updated the release notes, but I recommend allowing text to wrap by default. The previous behavior was broken from an accessibility perspective, so forcing text to not wrap can degrade the experience for many users. We don't consider this a breaking change because the previous experience was broken for many users.

Specifically in places where I also had an ion-input with the label, a label that was previously displaying fine, was now splitting into multiple lines even when there is plenty of room for the label.

@SaroGFX

SaroGFX commented Dec 13, 2023

Copy link
Copy Markdown

I built a lot of custom CSS upon the ion-item for our specific needs, and this minor update was definitely breaking a lot for us 😅 Maybe I shouldn't have used the ion-items in these custom UI's

Added a video to show some of the context where this is not desired. Is there a way to opt-out? I cannot find a way to overwrite because the changes are not exposed.

Screen.Recording.2023-12-13.at.15.59.33.mov

@liamdebeasi

Copy link
Copy Markdown
Contributor

You can set .ion-text-nowrap on elements you do not want to wrap: https://ionicframework.com/docs/api/item#basic-usage

@SaroGFX

SaroGFX commented Dec 14, 2023

Copy link
Copy Markdown

You can set .ion-text-nowrap on elements you do not want to wrap: https://ionicframework.com/docs/api/item#basic-usage

This utility does nothing to undo the changes to flex-wrap, flex-grow and shrink properties. It looks like I can't overwrite them either, because these elements seem to be unexposed. Is there any other workaround, besides rebuilding everything without using ion-item or wrapping it in a container?

Although an improvement, it would be great to be able to opt-out from this wrapping behaviour, as there are some scenarios where it's not desired. Maybe a custom property --flex-wrap could be a solution here?

@tgangso

tgangso commented Dec 14, 2023

Copy link
Copy Markdown

I also had some elements where we used ion-item as a base container for custom elements that I had to rewrite. Still do not agree this is not a breaking change. Found no workaround

@liamdebeasi

liamdebeasi commented Dec 14, 2023

Copy link
Copy Markdown
Contributor

Try setting width: 0 on the ion-label inside of the item. One of the primary changes was updating this element to flex: 1 0 auto:

Previously the flex-basis was 0 which defined the initial minimum size of the container. From there, the flex-grow value was 1 which meant the container could grow beyond 0.

The flex-basis was changed to auto which means the initial container size is now based on the width property, so setting width should allow you to retain the old flex-basis behavior.

Of course, if you get down to smaller widths such as 100px, the elements may still wrap/get truncated. However, I'd argue that not wrapping content at that small of a width makes the content hard to understand since most text will be not readable.

Here's a demo: https://codepen.io/liamdebeasi/pen/mdvZdjg

@liamdebeasi

Copy link
Copy Markdown
Contributor

Hi everyone,

The team is having discussions into revising part of the text wrapping behavior. In particular, we are considering not having the end/start slot content wrap to the next line. In other words, only the text in the default slot would wrap within its own container.

We are having this discussion based on some new information we've found related to how iOS handles wrapping. We're tracking this in #28769.

@kyleabens

Copy link
Copy Markdown

Is there anyway we can set the item-legacy class globally on all instances of ion-item and ion-item-divider without having to apply it to each one individually? Maybe add a property to IonicConfig for this so we can say:

IonicModule.forRoot({
      mode: 'md',
      itemLegacy: true
})

This recent change of wrapping ion-label by default does in a way break the UI of my app.

github-merge-queue Bot pushed a commit that referenced this pull request Jan 26, 2024
Issue number: resolves #28769

---------

<!-- 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. -->

As part of #28146, we
allowed text wrapping inside of `ion-item` for accessibility purposes.
One of the behaviors we added was to allow start, default, and end
slotted containers to wrap to the next line to align with the iOS spec.
However, this decision was based on an incorrect assumption.

The following screenshot shows the Settings app on iOS:

| default scale | 310% scale |
| - | - |
| <img width="1170" alt="Frame 4"
src="https://github.com/ionic-team/ionic-framework/assets/2721089/462ef153-a060-41c8-9a00-f0aad17839be">
| <img width="1170" alt="Frame 5"
src="https://github.com/ionic-team/ionic-framework/assets/2721089/f047f880-7b80-4710-939b-96da075fbbf9">
|

At the default scale, the blue icon is in the iOS equivalent of the
"start" slot, "Bluetooth" is in the default slot, and "On" is in the
"end" slot. We incorrectly assumed the same markup was true when scaling
the text up. However, at 310% scale the icon, "Bluetooth" text, and "On"
text all become part of the default slot in a single container that
wraps. You can tell because the bottom border runs underneath the blue
icon at 310% whereas it does not at the default scale. This allows the
text to wrap underneath the blue icon. When we originally implemented
#28146 we thought that this meant the start, default, and end slot
containers should wrap to the next line.

I further validated this behavior by creating an app with Swift UI. I
created a list of items where each item has the native equivalent of a
checkbox in the `start` slot and multiple `ion-labels` in the default
slot of the item:

| Default Scale | 310% Scale |
| - | - |
|
![IMG_3133](https://github.com/ionic-team/ionic-framework/assets/2721089/8b55bd1d-f7a8-4fec-bda4-d1bb12f50d34)
|
![IMG_3134](https://github.com/ionic-team/ionic-framework/assets/2721089/92e8a196-36e4-47d6-a4e5-a0e991c78d0d)
|

The content within each label wraps within the container, but the
containers themselves never wrap to the next line.

Demo code:

```swift
import SwiftUI

struct ContentView: View {
    struct Item: Identifiable, Hashable {
        let id = UUID()
    }

    private var items = [
        Item(),
        Item(),
        Item(),
        Item(),
        Item()
    ]

    @State private var multiSelection = Set<UUID>()

    var body: some View {
        NavigationView {
            List(items, selection: $multiSelection) {_ in
                HStack {
                    Text("Column 1 with really long text")
                    Text("Column 2 with really long text")
                    Text("Column 3 with really long text")
                    Text("Column 4 with really long text")
                }
            }
            .toolbar { EditButton() }
        }
        Text("\(multiSelection.count) selections")
    }
}

#Preview {
    ContentView()
}
```

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

- This PR removes the ability for the start, default, and end slot
containers to wrap to the next line. This behavior aligns with
pre-v7.6.0 behaviors. The containers inside of the default slot will not
wrap to the next line. However, content within each container (such as
text within an `ion-label`) will continue to wrap to meet the team's
accessibility requirements.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!--
  If this introduces a breaking change:
1. Describe the impact and migration path for existing applications
below.
  2. Update the BREAKING.md file with the breaking change.
3. Add "BREAKING CHANGE: [...]" to the commit description when merging.
See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#footer
for more information.
-->


## Other information

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

Dev build: `7.6.5-dev.11704916749.1e64a3a7`

---------

Co-authored-by: ionitron <hi@ionicframework.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: core @ionic/core package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants