Skip to content

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

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

Closed
4 of 6 tasks
astukanov opened this issue Dec 3, 2021 · 7 comments
Labels
package: core @ionic/core package type: bug a confirmed bug report

Comments

@astukanov
Copy link

astukanov commented Dec 3, 2021

Prerequisites

Ionic Framework Version

  • v4.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

@ionitron-bot ionitron-bot bot added the triage label Dec 3, 2021
@astukanov astukanov changed the title bug: bug: Angular ion-item combined with stacked ion-label loses item-label-stacked class once ion-chip is added Dec 3, 2021
@sean-perkins sean-perkins self-assigned this Dec 3, 2021
@sean-perkins
Copy link
Contributor

sean-perkins commented Dec 3, 2021

@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 sean-perkins added the needs: reply the issue needs a response from the user label Dec 3, 2021
@ionitron-bot ionitron-bot bot removed the triage label Dec 3, 2021
@sean-perkins sean-perkins removed their assignment Dec 3, 2021
@astukanov
Copy link
Author

astukanov commented Dec 12, 2021

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

@ionitron-bot ionitron-bot bot added triage and removed needs: reply the issue needs a response from the user labels Dec 12, 2021
@sean-perkins
Copy link
Contributor

@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 sean-perkins added package: core @ionic/core package type: bug a confirmed bug report labels Dec 14, 2021
@astukanov
Copy link
Author

astukanov commented Dec 14, 2021

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.

@liamdebeasi
Copy link
Contributor

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: #25661

@liamdebeasi
Copy link
Contributor

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.

@ionitron-bot
Copy link

ionitron-bot bot commented Feb 10, 2023

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 ionitron-bot bot locked and limited conversation to collaborators Feb 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
package: core @ionic/core package type: bug a confirmed bug report
Projects
None yet
Development

No branches or pull requests

3 participants