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

Slide-toggle fires change event on load #709

Closed
rafikk opened this issue Jun 17, 2016 · 9 comments · Fixed by #713
Closed

Slide-toggle fires change event on load #709

rafikk opened this issue Jun 17, 2016 · 9 comments · Fixed by #713

Comments

@rafikk
Copy link

rafikk commented Jun 17, 2016

Bug, feature request, or proposal:

Slide-toggle triggers change event on initialization if initial value of "checked" is true.

What is the expected behavior?

The event should only be fired in response to an interaction.

What is the current behavior?

The component fires the event before any user action has occurred.

What are the steps to reproduce?

Plunkr to reproduce here: http://plnkr.co/KCZtmelCWK6JKj0FDlh6

What is the use-case or motivation for changing an existing behavior?

If the app triggers an expensive operation, e.g. an API request, in response to the change event, this results in misfired updates and slows load time.

Which versions of Angular, Material, OS, browsers are affected?

All.

Is there anything else we should know?

I don't think so. :)

@devversion
Copy link
Member

I just confirmed that with master and it seems not to appear anymore.

@jelbourn
Copy link
Member

@devversion checkbox and radio use an _isInitialized flag to prevent this; slide-toggle might need to do the same

devversion added a commit to devversion/material2 that referenced this issue Jun 17, 2016
… multiple times

* Adds a test which confirms, that the slide-toggle isn't firing the (change) event multiple times.

Closes angular#709
@devversion
Copy link
Member

@jelbourn We already chatted about on the initial PR, but this bug is not noticeable in the md-slide-toggle component.

I've created a PR, which adds a test, which confirms that the change event isn't triggered multiple times after initialization and in general.

@rafikk
Copy link
Author

rafikk commented Jun 19, 2016

@devversion did you see the Plunkr I posted? The change event is definitely triggered on load in the example. I'm not referring to triggering the event "multiple" times, but rather triggering the event without user or program interaction.

@devversion
Copy link
Member

@rafikk I definitely see the issue. But the Plunker is running on Alpha.5 instead of master, which contains various changes.

@jelbourn
Copy link
Member

Closing based on @devversion's assessment.

@devversion
Copy link
Member

devversion commented Jun 20, 2016

@rafikk @jelbourn I'm sorry for the trouble. I was quite distracted this week.

I completely misunderstood the issue. This is still present in master

  • Thanks again @rafikk for the friendly ping.

I've updated the PR and added the correct fix (as done in the other components).

@jelbourn
Copy link
Member

Whoops, we mistook this for a different issue.

@jelbourn jelbourn reopened this Jun 20, 2016
jelbourn pushed a commit that referenced this issue Jun 21, 2016
* test(slide-toggle): add test to confirm that change event don't fires multiple times

* Adds a test which confirms, that the slide-toggle isn't firing the (change) event multiple times.

Closes #709

* update(): prevent change event to be fired at initialization

* update(): remove internal annotation
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants