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): correct click/slide event position #1303

Closed
wants to merge 1 commit into from
Closed

fix(slider): correct click/slide event position #1303

wants to merge 1 commit into from

Conversation

jmcgoldrick
Copy link

fixes #1234

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 22, 2016
@jmcgoldrick
Copy link
Author

The slider's dimensions were only being updated when created/resized. If the sliders position changes outside of these conditions the calculations for the thumb etc. were out.

It appears you only need one of the updates (either in onClick or onSlideStart) but I couldn't figure out why, also for consistency it seemed better to put it in both methods.

Is there a better way to account for position changes outside of the elements control?

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.

@jmcgoldrick can you add a unit test for this change?

@@ -182,6 +182,7 @@ export class MdSlider implements AfterContentInit, ControlValueAccessor {

this.isActive = true;
this.isSliding = false;
this._sliderDimensions = this._renderer.getSliderDimensions();
Copy link
Member

Choose a reason for hiding this comment

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

If this is only necessary in one handler, I would just leave it in the click handler. Also needs a comment explaining why the dimensions need to be recalculated here.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @jelbourn, two things:

  • to make the unit tests pass, _sliderDimensions needs to be updated in both onClick and onSlideStart, though it may be because the tests generate the click/slide events explicitly.
  • initially I couldn't write a failing test, I believe it is due to the way the click/slide events are generated in the tests.

The click/slide events in the unit tests take a "percentage", grab the up-to-date dimensions of the element and calculate the event coordinates based on that, which is backwards to how the browser does it. This means the values are always "correct" even if you move the sliders' parent around. See: https://github.com/angular/material2/blob/master/src/lib/slider/slider.spec.ts#L688

Adjusting these methods to take (pre-calculated) coordinates of an event, tests fail/pass as expected. Am I missing something obvious though...?

@chouclee
Copy link
Contributor

chouclee commented Oct 7, 2016

This PR also fixes #1389
The problem of #1389 is because when calling ngAfterContentInit(), _sliderDimensions would be an empty rectangle area (couldn't figure out why). Resizing the window will call this._renderer.getSliderDimensions() again to update dimensions.

@mmalerba
Copy link
Contributor

mmalerba commented Nov 3, 2016

This partially fixes the issue, but if the slider has an initial value it still won't show up properly. #1663 refactors this component to use percentage-based measurements, rather than pixel based ones for the thumb position which should catch this last edge case.

@jelbourn
Copy link
Member

jelbourn commented Nov 8, 2016

Superseded #1663

@jelbourn jelbourn closed this Nov 8, 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: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

md-slider not working inside md-sidenav
5 participants