fix(slider): refactor the slider to use percent values for the track … #1663

mmalerba · 2016-10-31T22:57:34Z

…fill and thumb position.

mmalerba · 2016-10-31T23:24:41Z

jelbourn

Overall looks good, just have some refactoring comments.

jelbourn · 2016-11-03T01:20:07Z

src/lib/slider/slider.html

+  <div class="md-slider-track-fill" [style.flexBasis]="percent * 100 + '%'"></div>
+  <div class="md-slider-ticks-container" [style.marginLeft]="-tickIntervalPercent / 2 * 100 + '%'">
+    <div class="md-slider-ticks" [style.marginLeft]="tickIntervalPercent / 2 * 100 + '%'"
+         [style.backgroundSize]="tickIntervalPercent * 100 + '% 2px'"></div>


Can we move these expressions into functions?

jelbourn · 2016-11-03T01:41:52Z

src/lib/slider/slider.scss

+.md-slider-ticks {
+  background: repeating-linear-gradient(to right, $md-slider-tick-color,
+      $md-slider-tick-color $md-slider-tick-size, transparent 0, transparent) repeat;
+  /* Firefox doesn't draw the gradient correctly with 'to right'


Use // style comments

jelbourn · 2016-11-03T01:44:58Z

src/lib/slider/slider.ts

+    '[attr.aria-valuemax]': 'max',
+    '[attr.aria-valuemin]': 'min',
+    '[attr.aria-valuenow]': 'value',
+


I don't think the blank lines are necessary in the host config

jelbourn · 2016-11-03T18:53:56Z

src/lib/slider/slider.ts

+  set step(v) {
+    // Only set the value to a valid number. v is casted to an any as we know it will come in as a
+    // string but it is labeled as a number which causes parseFloat to not accept it.
+    if (isNaN(parseFloat(<any> v))) {


Since this is used a lot, we should refactor to a common function like coerceNumberProperty, something like...

export function coerceNumberProperty(value: any, fallbackValue = 0) { return isNaN(parseFloat(v as any)) ? fallbackValue : Number(v); )

This would be a slight change, since it will go back to the default when getting an invalid value, but I think that is more sensible behavior than not updating at all.

jelbourn · 2016-11-03T18:56:49Z

src/lib/slider/slider.ts

-    // Only set the value to a valid number. v is casted to an any as we know it will come in as a
-    // string but it is labeled as a number which causes parseFloat to not accept it.
-    if (isNaN(parseFloat(<any> v))) {
+  /** TODO: internal */


You can omit the TODO: internal comments now

jelbourn · 2016-11-03T18:58:22Z

src/lib/slider/slider.ts

-    this._controlValueAccessorChangeFn(this.value);
-    this.snapThumbToValue();
-    this._updateTickSeparation();
+    this._updateTickIntervalPercent();


Add comment saying why you update dimensions and ticks on mouseenter

jelbourn · 2016-11-03T19:01:16Z

src/lib/slider/slider.ts

-    this._renderer.updateThumbAndFillPosition(this._percent, this._sliderDimensions.width);
+    setTimeout(() => {
+      this.isSliding = false;
+    }, 0);


You can use requestAnimationFrame

requestAnimationFrame(() => this.isSliding = false);

I think this is not even necessary anymore now that the position is percent based.

jelbourn · 2016-11-03T19:04:23Z

stylelint-config.json

@@ -28,7 +28,9 @@

    "property-case": "lower",

-    "declaration-block-no-duplicate-properties": true,
+    "declaration-block-no-duplicate-properties": [ true, {
+      "ignore": ["consecutive-duplicates"]


Let's use consecutive-duplicates-with-different-values in case someone just accidentally duplicates a line.

So I actually tried that and I got: Invalid value "consecutive-duplicates-with-different-values", maybe we're not using the latest version of stylelint?

…fill and thumb position.

mmalerba · 2016-11-04T00:27:36Z

Comments addressed and rebased

jelbourn

Just a couple last comments

jelbourn · 2016-11-07T17:48:42Z

src/lib/core/coersion/number-property.ts

@@ -0,0 +1,4 @@
+/** Coerces a data-bound value (typically a string) to a number. */
+export function coerceNumberProperty(value: any, fallbackValue = 0) {
+  return isNaN(parseFloat(value as any)) ? fallbackValue : Number(value);


On second look I realize that parseFloat(value) and Number(value) are subtly different in their handling of non-numeric characters. E.g.,

parseFloat('123 boats') == 123 Number('123 boats') === NaN

We should just use one of these- probably Number:

let numberValue = Number(value); return isNaN(numberValue) ? fallbackValue : numberValue;

jelbourn · 2016-11-07T17:52:37Z

src/lib/slider/slider.ts

+  set value(v: number) {
+    this._value = coerceNumberProperty(v, this._value);
+    this._percent = this._calculatePercentage(this._value);
+    this._controlValueAccessorChangeFn(this._value);


We should only invoke the controlValueAccessorChangeFn when the value was changed via user interaction, though we could change this in a separate PR since it was already ike this.

fixes angular#1389 fixes angular#1304 fixes angular#1234

angular-automatic-lock-bot · 2019-09-06T04:02:19Z

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.}

mmalerba added the pr: needs review label Oct 31, 2016

mmalerba assigned jelbourn Oct 31, 2016

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 31, 2016

mmalerba mentioned this pull request Nov 3, 2016

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

Closed

jelbourn reviewed Nov 3, 2016

View reviewed changes

mmalerba added 8 commits November 3, 2016 16:53

fix(slider): refactor the slider to use percent values for the track …

6dd63d5

…fill and thumb position.

Fix lint error.

dccac45

Fix tick position on firefox.

dda95a4

Crisper ticks on firefox.

9a03857

Allow consecutive duplicate properties in SCSS.

31eb479

Addressed comments.

42ed436

PercentPipe was adding extra space before '%', so replaced it.

a4ba485

Fixed tests.

f3604cf

mmalerba force-pushed the slider-pr branch from c33726b to f3604cf Compare November 4, 2016 00:15

Fix tab group on slider demo page.

565a6f3

remove CommonModule from imports.

d5fb4e7

mmalerba mentioned this pull request Nov 7, 2016

fix(slider): keyboard support #1759

Merged

jelbourn reviewed Nov 8, 2016

View reviewed changes

Addressed comments.

c48f7d6

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Nov 8, 2016

jelbourn merged commit 8815846 into angular:master Nov 8, 2016

rolandjitsu pushed a commit to rolandjitsu/material2 that referenced this pull request Nov 10, 2016

fix(slider): use percent values for the track (angular#1663)

fa14453

fixes angular#1389 fixes angular#1304 fixes angular#1234

mmalerba mentioned this pull request Nov 15, 2016

If md-slider changes position after initialization, it can't be clicked in the new location #1845

Closed

mmalerba deleted the slider-pr branch November 18, 2016 00:34

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(slider): refactor the slider to use percent values for the track … #1663

fix(slider): refactor the slider to use percent values for the track … #1663

mmalerba commented Oct 31, 2016

mmalerba commented Oct 31, 2016 •

edited

Loading

jelbourn left a comment

jelbourn Nov 3, 2016

jelbourn Nov 3, 2016

jelbourn Nov 3, 2016

jelbourn Nov 3, 2016

jelbourn Nov 3, 2016

jelbourn Nov 3, 2016

jelbourn Nov 3, 2016

mmalerba Nov 3, 2016

jelbourn Nov 3, 2016

mmalerba Nov 3, 2016

mmalerba commented Nov 4, 2016

jelbourn left a comment

jelbourn Nov 7, 2016 •

edited

Loading

jelbourn Nov 7, 2016

angular-automatic-lock-bot bot commented Sep 6, 2019

fix(slider): refactor the slider to use percent values for the track … #1663

fix(slider): refactor the slider to use percent values for the track … #1663

Conversation

mmalerba commented Oct 31, 2016

mmalerba commented Oct 31, 2016 • edited Loading

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmalerba commented Nov 4, 2016

jelbourn left a comment

Choose a reason for hiding this comment

jelbourn Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Sep 6, 2019

mmalerba commented Oct 31, 2016 •

edited

Loading

jelbourn Nov 7, 2016 •

edited

Loading