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

fix(slider): don't emit change events on mousedown #20240

Merged

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Aug 7, 2020

Currently when the user presses down somewhere on the slider we emit a change event, as well as when they release. While we don't emit the same value twice, the timing seems to be unexpected by consumers, because one happens at the begging of the sequence and another one at the end.

These changes make it so that we only emit the change event once the user has released the slider.

Fixes #14363.

Note: I'm setting it to P2, because it's a long standing issue that people keep bumping into.

@crisbeto crisbeto added P2 The issue is important to a large percentage of users, with a workaround merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: patch This PR is targeted for the next patch release labels Aug 7, 2020
@crisbeto crisbeto requested a review from mmalerba as a code owner August 7, 2020 20:01
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 7, 2020
@crisbeto
Copy link
Member Author

crisbeto commented Aug 7, 2020

Caretaker note: this has the potential to break people's tests, as evidenced by the changes I had to make to our own tests. We should see how break-ey this is before we invest a lot of time trying to get it synced in.

Currently when the user presses down somewhere on the slider we emit a `change` event, as well as when they release. While we don't emit the same value twice, the timing seems to be unexpected by consumers, because one happens at the begging of the sequence and another one at the end.

These changes make it so that we only emit the change event once the user has released the slider.

Fixes angular#14363.
@crisbeto crisbeto force-pushed the 14363/slider-change-event-mousedown branch from 3651231 to 0b3020e Compare August 7, 2020 20:16
@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label Aug 11, 2020
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

LGTM

@andrewseguin andrewseguin merged commit ce72369 into angular:master Aug 14, 2020
andrewseguin pushed a commit that referenced this pull request Aug 14, 2020
Currently when the user presses down somewhere on the slider we emit a `change` event, as well as when they release. While we don't emit the same value twice, the timing seems to be unexpected by consumers, because one happens at the begging of the sequence and another one at the end.

These changes make it so that we only emit the change event once the user has released the slider.

Fixes #14363.

(cherry picked from commit ce72369)
@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 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note P2 The issue is important to a large percentage of users, with a workaround target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MatSlider: change event fired twice
4 participants