-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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): Update slider position and ticks when the min and max #1598
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs unit tests for updating min/max.
@@ -265,14 +266,28 @@ export class MdSlider implements AfterContentInit, ControlValueAccessor { | |||
*/ | |||
snapThumbToValue() { | |||
this.updatePercentFromValue(); | |||
this._renderer.updateThumbAndFillPosition(this._percent, this._sliderDimensions.width); | |||
if (this._sliderDimensions) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which cases does this._sliderDimensions
not exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When min or max is set before ngAfterContentInit.
@@ -425,8 +440,10 @@ export class SliderRenderer { | |||
* Draws ticks onto the tick container. | |||
*/ | |||
drawTicks(tickSeparation: number) { | |||
let sliderTrackContainer = | |||
<HTMLElement>this._sliderElement.querySelector('.md-slider-track-container'); | |||
let tickContainerWidth = sliderTrackContainer.getBoundingClientRect().width; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the size calculation for the tick container change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was checking the width of the same container whose width it was setting. This wasn't a problem before since it never changed after initialization, but now it can change.
@@ -444,6 +461,8 @@ export class SliderRenderer { | |||
// separation), don't show it by decreasing the width of the tick container element. | |||
if (tickContainerWidth % tickSeparation < (tickSeparation / 2)) { | |||
tickContainer.style.width = tickContainerWidth - tickSeparation + 'px'; | |||
} else { | |||
tickContainer.style.width = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add comment here like
// If there is enough space for the second-to-last tick, restore the default width of the tick container.
* Initializes the tick and slider positions. | ||
* @private | ||
*/ | ||
private _initTicksAndSlider() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I think it would be better to call the individual functions in the setters / init because it's more clear in those locations what's happening; you won't know what _initTicksAndSlider
does precisely without jumping to the definition.
|
||
expect(trackFillDimensions.width).toBe(sliderDimensions.width * 6 / 8); | ||
expect(tickContainerDimensions.width) | ||
.toBe(sliderDimensions.width - sliderDimensions.width * 6 / 8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we also need to check that the linear gradient background for the ticks has been updated (since the tick interval will have changed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Looks like there are some test failures on CI: |
1cbe4e0
to
d5333b6
Compare
LGTM |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
values change. Closes #1556