Skip to content

Fix: dispatching change event after new value is set. #27

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
wants to merge 5 commits into from
Closed

Fix: dispatching change event after new value is set. #27

wants to merge 5 commits into from

Conversation

Sachin-chaurasiya
Copy link

@Sachin-chaurasiya Sachin-chaurasiya commented Feb 10, 2022

@keithamus, Please review the PR.

@Sachin-chaurasiya Sachin-chaurasiya requested a review from a team as a code owner February 10, 2022 14:51
@@ -129,6 +129,8 @@ class TextExpander {

this.input.value = beginning + value + remaining

this.input.dispatchEvent(new Event('change', {bubbles: true}))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please add a test that confirms this behaviour, that we can also use to validate and avoid regressions here.

Copy link
Author

Choose a reason for hiding this comment

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

@keithamus , I don't have experience in testing. can you please help me out here?

Copy link
Author

Choose a reason for hiding this comment

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

@keithamus, I have added some tests by looking at previous tests.

@Sachin-chaurasiya
Copy link
Author

@keithamus , please review this PR.

Copy link
Contributor

@koddsson koddsson left a comment

Choose a reason for hiding this comment

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

Hey @Sachin-chaurasiya; the tests run fine for me when I remove the line that dispatches the change event.

Maybe you can add motivation for this change to the PR and double-check that this change event is being dispatched already by the browser.

@@ -129,6 +129,8 @@ class TextExpander {

this.input.value = beginning + value + remaining

this.input.dispatchEvent(new Event('change', {bubbles: true}))
Copy link
Contributor

Choose a reason for hiding this comment

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

The tests run green for me with this line removed so either the tests are wrong or the implementation.

@Sachin-chaurasiya
Copy link
Author

Closing this PR as an Issue is different.

@Sachin-chaurasiya Sachin-chaurasiya deleted the input-dispatch-event branch April 3, 2022 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants