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): constraints value between min and max #1567

Closed
wants to merge 31 commits into from

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Oct 22, 2016

Should fix #1557

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 22, 2016
@@ -151,7 +151,7 @@ export class MdSlider implements AfterContentInit, ControlValueAccessor {
return;
}

this._value = Number(v);
this._value = Math.min(Math.max(Number(v), this.min), this.max);
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking that it makes more sense to clamp the rendered position of the thumb than the actual value. Having an invalid value is a state that the control should allow, where the user will likely want to show a validation error.

An easy way to do this would be to have a getRenderedValue method that did the clamping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jelbourn I've created a new patch in this idea.

@jelbourn
Copy link
Member

cc @mmalerba

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

Just a few last minor comments


it('should place the thumb on the min value', () => {
thumbDimensions = thumbElement.getBoundingClientRect();
expect(thumbDimensions.left).toBe(sliderDimensions.width * 0.0 + sliderDimensions.left);
Copy link
Member

Choose a reason for hiding this comment

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

This can just be

expect(thumbDimensions.left).toBe(sliderDimensions.left);


it('should place the thumb on the max value', () => {
thumbDimensions = thumbElement.getBoundingClientRect();
expect(thumbDimensions.left).toBe(sliderDimensions.width * 1.0 + sliderDimensions.left);
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to do

expect(thumbDimensions.left).toBe(sliderDimensions.right);

});
});

describe('slider with set min and max and a value between min and max', () => {
Copy link
Member

Choose a reason for hiding this comment

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

This behavior is already captured by existing unit tests in the "slider with set min and max" section.

styles: [noTransitionStyle],
encapsulation: ViewEncapsulation.None
})
class SliderWithValueBetweenMinAndMax { }
Copy link
Member

Choose a reason for hiding this comment

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

You don't need this component any more.

@jelbourn
Copy link
Member

LGTM, thanks!

@jelbourn
Copy link
Member

@feloy Looks like this just needs another rebase

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

1 similar comment
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Oct 26, 2016
@feloy
Copy link
Contributor Author

feloy commented Oct 26, 2016

Is my rebase ok? I'm not sure if I have to do something about cla:no?

@jelbourn
Copy link
Member

@feloy something seems to have gone amiss with the rebase; PR is showing 31 commits

@feloy
Copy link
Contributor Author

feloy commented Oct 26, 2016

@jelbourn Yes I think I missed my rebase. I can close this PR and create a new one. ok?

@jelbourn
Copy link
Member

@feloy Fine by me, whatever is easiest for you

@feloy
Copy link
Contributor Author

feloy commented Oct 26, 2016

Replaced by #1617

@feloy feloy closed this Oct 26, 2016
@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 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla
Projects
None yet
Development

Successfully merging this pull request may close these issues.

md-slider distortion when value bigger than min or max is assigned
8 participants