Skip to content

Conversation

@llorenspujol
Copy link
Contributor

Currently, the slider is firing a change event on slideEnd when it is disabled. If it is disabled and the value of the slider doesn't change, it shouldn't have to emit any change event.

I have added the test and its corresponding fix for it to pass.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 17, 2018
this._isSliding = false;

if (this._valueOnSlideStart != this.value) {
if (this._valueOnSlideStart != null && this._valueOnSlideStart != this.value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is quite right, how about: this._valueOnSlideStart != this.value && !this.disabled

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, maybe is better, I am not in context at all. I will make the change (Y)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made the change @mmalerba

@llorenspujol llorenspujol force-pushed the fix-slider-change-when-disabled branch from 1549883 to 7ed6b37 Compare January 18, 2018 08:04
@mmalerba mmalerba added pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jan 19, 2018
@andrewseguin
Copy link
Contributor

Got a minor lint error:

ERROR: /home/travis/build/angular/material2/src/lib/slider/slider.spec.ts[257, 1]: trailing whitespace

@mmalerba mmalerba removed the action: merge The PR is ready for merge by the caretaker label Jan 23, 2018
@jelbourn
Copy link
Member

This passes google presubmit

@llorenspujol we can merge this if you correct the lint error

Currently the slider is firing a change event on slideEnd when it is disabled. If it is disabled and the value of the slider doesn't change, it shouldn't have to emit any change event.
@llorenspujol llorenspujol force-pushed the fix-slider-change-when-disabled branch from 7ed6b37 to 0d1fbe2 Compare January 25, 2018 10:13
@llorenspujol
Copy link
Contributor Author

@jelbourn @andrewseguin lint fixed.

@jelbourn jelbourn merged commit 1e2fe90 into angular:master Jan 25, 2018
@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 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants