Skip to content
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

feat(textarea): ionChange will only emit from user committed changes #25953

Merged
merged 18 commits into from
Sep 23, 2022

Conversation

sean-perkins
Copy link
Contributor

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
    • Some docs updates need to be made in the ionic-docs repo, in a separate PR. See the contributing guide for details.
  • Build (npm run build) was run locally and any changes were pushed
  • Lint (npm run lint) has passed locally and any fixes were made for failures

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Other (please describe):

What is the current behavior?

Textarea emits ionChange each time the value is modified internally (e.g. user typing) or externally (e.g. value being set programmatically).

Issue URL: Internal

What is the new behavior?

  • ionChange is only emitted when the inner textarea change event is fired.
  • debounce applies to the ionInput event instead of ionChange

Does this introduce a breaking change?

  • Yes
  • No

Other information

@github-actions github-actions bot added package: angular @ionic/angular package package: core @ionic/core package labels Sep 16, 2022
@sean-perkins sean-perkins changed the title Fw 2038 feat(textarea): ionChange will only emit from user committed changes Sep 16, 2022
@sean-perkins sean-perkins marked this pull request as ready for review September 19, 2022 15:29
@sean-perkins sean-perkins requested a review from a team as a code owner September 19, 2022 15:29
@@ -35,6 +35,7 @@ export class TextValueAccessorDirective extends ValueAccessor {
},
],
})
// TODO rename this value accessor to `TextValueAccessorDirective` when search-bar is updated
Copy link
Contributor

Choose a reason for hiding this comment

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

How come? Wouldn't we just get rid of the TextValueAccessorDirective class/directive altogether and be done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing TextValueAccessorDirective would introduce a breaking change to the Angular package, which we don't need to make. The "new" value accessor only exists in the span of migrating the controls to using ionInput instead of ionChange, then it can assume the existing name.

core/src/components/textarea/textarea.tsx Outdated Show resolved Hide resolved
@amandaejohnston amandaejohnston requested a review from a team September 19, 2022 18:21
@amandaejohnston amandaejohnston requested a review from a team September 19, 2022 20:23
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.

Given <ion-textarea clear-on-edit="true" value="123"></ion-textarea>, ionChange does not emit when pressing backspace (After blurring).

ionInput does not fire either, but that seems to be the correct behavior since Ionic is clearing the input, not the user. The current textarea does this too. (might be worth mentioning in the docs) Should ionInput fire because the user pressed backspace?

core/src/components/textarea/test/textarea-events.e2e.ts Outdated Show resolved Hide resolved
core/src/components/textarea/textarea.tsx Show resolved Hide resolved
core/src/components/textarea/textarea.tsx Outdated Show resolved Hide resolved
@sean-perkins
Copy link
Contributor Author

@liamdebeasi couple of call outs from the last changes + feedback to your questions.

Given <ion-textarea clear-on-edit="true" value="123"></ion-textarea>, ionChange does not emit when pressing backspace (After blurring).

This has been fixed. This appears to be a bug in the clearOnEdit implementation. It does not clear the textarea if the element has an initial value, unless it is focused, blurred, focused and then modified. I have modified this to clear on edit when the control has a value.

Should ionInput fire because the user pressed backspace?

I do not believe so. We aren't stopping event propagation here. The default behavior with a textarea without a value, when pressing backspace, is to not emit an input event: https://codepen.io/sean-perkins-ionic/pen/gOzxGGQ. With the input value being cleared on edit, we would have to manually emit an ionInput event.


Other misc call outs:

  • clearOnEdit was mutable, even though its value was never internally modified (removed the mutable flag)
  • Property documentation kept referring to type being specified, but it has no effect on the ion-textarea implementation - removed this incorrect documentation.

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.

Great job!

core/src/components/textarea/textarea.tsx Show resolved Hide resolved
@sean-perkins sean-perkins merged commit 68bae80 into feature-7.0 Sep 23, 2022
@sean-perkins sean-perkins deleted the FW-2038 branch September 23, 2022 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package package: core @ionic/core package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants