ionic-framework icon indicating copy to clipboard operation
ionic-framework copied to clipboard

bug: Angular ion-item combined with stacked ion-label loses item-label-stacked class once ion-chip is added

Open astukanov opened this issue 2 years ago • 5 comments

Prerequisites

Ionic Framework Version

  • [ ] v4.x
  • [X] v5.x
  • [ ] v6.x

Current Behavior

ion-item combined with stacked ion-label loses item-label-stacked class once ion-chip is added inside ion-item.

Expected Behavior

ion-item stays in stacked mode after ion-chip is added

Steps to Reproduce

  • Create ion-item with input and add chip elements which will be populated once input changes.
  • Once an ion-chip is added inside the ion-item element the ion-item loses the item-label-stacked class.

Code Reproduction URL

https://github.com/astukanov/ion-label-stacked-with-chip-bug

Ionic Info

No response

Additional Information

No response

astukanov avatar Dec 03 '21 11:12 astukanov

@astukanov thanks for the bug report and reproduction app!

The issue here is the use of multiple ion-label that re-render in an ion-item. When ion-label first renders, it emits an event to update the ion-item with the correct CSS classes based on the label's position.

With the usage of *ngFor you are causing a new ion-label to render, which notifies ion-item that an ion-label without a position="stacked" has rendered and this removes the class.

<ion-item>
  <ion-label position="stacked">Label</ion-label>
  <ion-chip *ngFor="let value of control.value">
    <ion-label>{{value}}</ion-label>
  </ion-chip>
</ion-item>

To fix your problem, update the ion-label to any other tag (span or p).

<ion-chip *ngFor="let value of control.value">
  <span>{{value}}</span>
</ion-chip>

Can you let me know if this solves the problem on your end?

sean-perkins avatar Dec 03 '21 16:12 sean-perkins

Hi @sean-perkins

Thanks a lot for your reply and support. Yes, avoiding the use of ion-label inside chips works like a charm.

Would it maybe make sense to change the examples at https://ionicframework.com/docs/api/chip to avoid such incidents? I have also noticed that the color is not set dynamically anymore when using span instead of ion-label. Can this bug stay active in that case or should this be translated into a feature request?

P.S. for those who are interested if adding the stacked property to ion-label inside the chips will sove the issue, here is what happens:

Code:

<ion-item>
  <ion-label position="stacked">Label</ion-label>
  <ion-chip *ngFor="let value of control.value">
    <ion-label position="stacked">{{value}}</ion-label>
  </ion-chip>
</ion-item>

Result: image

astukanov avatar Dec 12 '21 17:12 astukanov

@astukanov Really good question regarding the documentation. I'll consult with the team and discuss best options for this issue.

There's related issues around ion-item and ion-label that highlights other flaws that exist in the current implementation pattern (#22541).

The relationship of how the inner ion-label can alter the outer ion-item, results in a small decrease in performance with re-rendering and also causes related side effects such as this bug; where the last rendered ion-label overrides the ion-item completely.

I've started to explore this issue privately and there's a few paths that have presented themselves (although I am open to suggestions):

  1. Only allow the first child that matches ion-label to modify the ion-item. This would allow multiple ion-label, but could cause odd behaviors that aren't immediately clear to the implementer.
<ion-item>
   <ion-label position="stacked">Email</ion-label>
   <ion-input></ion-input>
   <ion-label>Because this is last it will not affect ion-item.</ion-label>
</ion-item>
  1. Introduce a new property for ion-label, such as main that would allow the implementer to hard-set the label that is the main label for the item's container.
<ion-item>
   <ion-label position="stacked" main="true">Email</ion-label>
   <ion-input></ion-input>
   <ion-label>Your email must contain a .com</ion-label>
</ion-item>
  1. Have explicit variants of ion-item that removes the need for implementers to maintain the ion-label themselves, such as:
<ion-stacked-item label="Plain text">
   <ion-input></ion-input>
</ion-stacked-item>

<ion-stacked-item color="secondary">
   <div slot="label">
        Slot usage of label
   </div>
   <ion-input></ion-input>
</ion-stacked-item>
  1. Extend ion-item's API to allow specifying the main label ID that is used for the implementation. Ionic could generate the label ID automatically (a consistent generated value) or you could use your own (preferred).
<ion-item labelId="emailLbl">
   <ion-label id="emailLbl" position="stacked">Email</ion-label>
   <ion-input></ion-input>
</ion-item>

Option 3 is currently the only path that resolves the inner contents, automatically re-rendering the ion-item on first paint, but is also the largest change for Ionic developers who have been using ion-item for many years.

sean-perkins avatar Dec 12 '21 23:12 sean-perkins

Hi @sean-perkins Very Interesting proposals. Thanks!

To be honest I'm very new to Ionic and havent had the possibility yet to deep-dive into the framework.

From my experience option 3, alongside the mentioned disadvantages, would add further complexity to the framework. It would be great to avoid this if possible.

The proposals (2 and 4), are so far my favorites. they allow to keep the current implemetation as is and provide additional functionality while also being robust.

To achieve this without blocking the id attribute or to prevent multiple DOM child check iterations, an ion-label could be passed per reference to ion-item. If a reference is set, it would ignore events from other ion-label elements and prevent interferance with desireed settings.

Example:

  <ion-item [label]="label">
     <ion-label #label position="stacked">Email</ion-label>
     <ion-input></ion-input>
  </ion-item>

Another solution could be to extract a base component from ion-label, then create two subcomponents from it: ion-label with the exact same behavior it has now, and an ion-label-secondary | ion-tag | ion-stamp... which also extends it and has just a subset of theion-label functionality e.g. does not propagate events to the ion-item component.

astukanov avatar Dec 14 '21 17:12 astukanov

Hi there,

We are proposing some changes to form components that seek to resolve this issue. We would love your feedback on these proposed changes! You can read more about the changes and leave comments here: https://github.com/ionic-team/ionic-framework/discussions/25661

liamdebeasi avatar Aug 02 '22 21:08 liamdebeasi

Thanks for the report. In Ionic 7 we are introducing a new syntax for working with ion-input. This issue is fixed when using that new syntax. We will have an Ionic 7 beta in the future where developers can test and provide feedback. There will also be a migration guide to show how to update to the new input syntax.

The work required to resolve this has been completed, so I am going to close this thread. Let me know if there are any questions.

liamdebeasi avatar Jan 11 '23 21:01 liamdebeasi

Thanks for the issue! This issue is being locked to prevent comments that are not relevant to the original issue. If this is still an issue with the latest version of Ionic, please create a new issue and ensure the template is fully filled out.

ionitron-bot[bot] avatar Feb 10 '23 22:02 ionitron-bot[bot]