Skip to content

fix(many): dynamic label support for modern form controls #27156

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

Merged
merged 11 commits into from
Apr 17, 2023
Merged

Conversation

sean-perkins
Copy link
Contributor

@sean-perkins sean-perkins commented Apr 10, 2023

What is the current behavior?

Developers that are using Ionic v7 are experiencing an issue where implementations that are intended to use the modern control syntax will render with the legacy syntax and a warning will be displayed.

The issue is most easily reproduced by not assigning a label to the control and then asynchronously assigning a label after a duration.

Angular example:

<ion-item>
  <ion-input [label]="label"></ion-input>
</ion-item>
@Component({ ... })
export class MyComponent {
  @Input() label?: string; // initially unset

  ngOnInit() {
    setTimeout(() => this.label = 'Hello world', 500);
  }
}

Issue URL: resolves #27085

What is the new behavior?

  • Form controls that do not have a decorative label or aria-label/aria-labelledby assigned, will default render as modern controls.
    • Legacy form implementations that render an <ion-label> within the item, will continue to render with the legacy template and a warning will be displayed in the console.
  • Modern form syntax supports dynamically set labels

Does this introduce a breaking change?

  • Yes
  • No

Legacy implementations that do not have a decorative label and do not specify aria-label on the control, will be upgraded to the modern syntax.

For example:

<ion-item>
  <ion-input></ion-input>
</ion-item>

Developers that do not want to update to the modern syntax yet should add the legacy="true" attribute to their form control.

Other information

Dev-build: 7.0.2-dev.11681157690.1060bc7f

When migrating the range tests to modern syntax, I observed a visual clipping issue. This is being addressed in: #27188. This PR simply adds the legacy flag so that screenshots are the same as main.

@bolt-new-by-stackblitz
Copy link

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@github-actions github-actions bot added package: angular @ionic/angular package package: core @ionic/core package labels Apr 10, 2023
sean-perkins added a commit that referenced this pull request Apr 12, 2023
<!-- Please refer to our contributing documentation for any questions on
submitting a pull request, or let us know here if you need any help:
https://ionicframework.com/docs/building/contributing -->

<!-- Some docs updates need to be made in the `ionic-docs` repo, in a
separate PR. See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#modifying-documentation
for details. -->

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

There are tests for `ion-input` that are not in the `/legacy` test
folder, that are using invalid modern syntax.

Inversely, there is legacy tests that do not explicitly set
`legacy="true"` that will be updated the modern syntax when
#27156 is merged.

<!-- Issues are required for both bug fixes and features. -->
Issue URL: N/A


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

- Updates modern tests to use modern label syntax (using `aria-label`)
- Applies `legacy="true"` to legacy test templates
- Fixes a typo in a legacy test where the selector was incorrect

## 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>
@github-actions github-actions bot removed the package: angular @ionic/angular package label Apr 17, 2023
@sean-perkins sean-perkins marked this pull request as ready for review April 17, 2023 02:19
@sean-perkins sean-perkins requested a review from a team as a code owner April 17, 2023 02:19
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

There are quite a few tests using the legacy syntax that should be using the modern syntax. Is this intentional? My understanding of #27199 is this only impacts ranges used inside of ion-item, but these impacted ranges are not being used in an item.

liamdebeasi pushed a commit that referenced this pull request Apr 17, 2023
<!-- Please refer to our contributing documentation for any questions on
submitting a pull request, or let us know here if you need any help:
https://ionicframework.com/docs/building/contributing -->

<!-- Some docs updates need to be made in the `ionic-docs` repo, in a
separate PR. See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#modifying-documentation
for details. -->

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

There are tests for `ion-input` that are not in the `/legacy` test
folder, that are using invalid modern syntax.

Inversely, there is legacy tests that do not explicitly set
`legacy="true"` that will be updated the modern syntax when
#27156 is merged.

<!-- Issues are required for both bug fixes and features. -->
Issue URL: N/A


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

- Updates modern tests to use modern label syntax (using `aria-label`)
- Applies `legacy="true"` to legacy test templates
- Fixes a typo in a legacy test where the selector was incorrect

## 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>
@sean-perkins sean-perkins added this pull request to the merge queue Apr 17, 2023
Merged via the queue into main with commit 30b548b Apr 17, 2023
@sean-perkins sean-perkins deleted the FW-3891 branch April 17, 2023 20:48
liamdebeasi pushed a commit that referenced this pull request Apr 19, 2023
<!-- Please refer to our contributing documentation for any questions on
submitting a pull request, or let us know here if you need any help:
https://ionicframework.com/docs/building/contributing -->

<!-- Some docs updates need to be made in the `ionic-docs` repo, in a
separate PR. See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#modifying-documentation
for details. -->

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

There are tests for `ion-input` that are not in the `/legacy` test
folder, that are using invalid modern syntax.

Inversely, there is legacy tests that do not explicitly set
`legacy="true"` that will be updated the modern syntax when
#27156 is merged.

<!-- Issues are required for both bug fixes and features. -->
Issue URL: N/A


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

- Updates modern tests to use modern label syntax (using `aria-label`)
- Applies `legacy="true"` to legacy test templates
- Fixes a typo in a legacy test where the selector was incorrect

## 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>
liamdebeasi pushed a commit that referenced this pull request Apr 19, 2023
<!-- Please refer to our contributing documentation for any questions on
submitting a pull request, or let us know here if you need any help:
https://ionicframework.com/docs/building/contributing -->

<!-- Some docs updates need to be made in the `ionic-docs` repo, in a
separate PR. See
https://github.com/ionic-team/ionic-framework/blob/main/.github/CONTRIBUTING.md#modifying-documentation
for details. -->

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

Developers that are using Ionic v7 are experiencing an issue where
implementations that are intended to use the modern control syntax will
render with the legacy syntax and a warning will be displayed.

The issue is most easily reproduced by not assigning a label to the
control and then asynchronously assigning a label after a duration.

Angular example:
```html
<ion-item>
  <ion-input [label]="label"></ion-input>
</ion-item>
```

```ts
@component({ ... })
export class MyComponent {
  @input() label?: string; // initially unset

  ngOnInit() {
    setTimeout(() => this.label = 'Hello world', 500);
  }
}
```

<!-- Issues are required for both bug fixes and features. -->
Issue URL: resolves #27085


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

- Form controls that do not have a decorative label or
`aria-label`/`aria-labelledby` assigned, will default render as modern
controls.
- Legacy form implementations that render an `<ion-label>` within the
item, will continue to render with the legacy template and a warning
will be displayed in the console.
- Modern form syntax supports dynamically set labels

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

Legacy implementations that do not have a decorative label and do not
specify `aria-label` on the control, will be upgraded to the modern
syntax.

For example:
```html
<ion-item>
  <ion-input></ion-input>
</ion-item>
```

Developers that do not want to update to the modern syntax yet should
add the `legacy="true"` attribute to their form control.

## 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.0.2-dev.11681157690.1060bc7f`

When migrating the range tests to modern syntax, I observed a visual
clipping issue. This is being addressed in:
#27188. This PR simply
adds the legacy flag so that screenshots are the same as `main`.
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.

bug: v7, cannot pass dynamic label to form controls
2 participants