Material WebUI: cr-slider element for intelligent range mapping

ui/webui/resources/cr_elements/cr_slider/cr_slider.js

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+/**
+ * @fileoverview
+ * cr-slider wraps a paper-slider. It maps the slider's values from a linear UI
+ * range to a range of real values.  When |value| does not map exactly to a
+ * tick mark, it interpolates to the nearest tick.
+ *
+ * Unlike paper-slider, there is no distinction between value and
+ * immediateValue; when either changes, the |value| property is updated.
+ */
+Polymer({
+  is: 'cr-slider',
+
+  properties: {
+    /** The value the slider represents and controls. */
+    value: {
+      type: Number,
+      notify: true,
+      observer: 'valueChanged_',
+    },
+
+    /** @type {!Array<number>} Values corresponding to each tick. */
+    tickValues: Array,
+
+    disabled: {
+      type: Boolean,
+      value: false,
+      reflectToAttribute: true,
+    },
+
+    snaps: {
+      type: Boolean,
+      value: false,
+    },
+
+    maxMarkers: Number,

[Dan Beam, 2016/05/17 19:35:45]

where is this used?

[michaelpg, 2016/05/18 17:05:49]

html for paper-slider

+  },
+
+  observers: [
+    'tickValuesChanged_(tickValues.*)',
+  ],
+
+  /**
+   * Sets the |value| property to the value corresponding to the knob position
+   * after a user action.
+   * @private
+   */
+  onSliderChange_: function() {
+    var index = this.$.slider.immediateValue;
+    this.value = this.tickValues[index];

[Dan Beam, 2016/05/17 19:35:45]

is this less clear?

this.value = this.tickValues[this.$.slider.immediateValue];

[michaelpg, 2016/05/18 17:05:48]

Done.

+  },
+
+  /**
+   * Updates the slider settings and knob position for the new tick values.
+   * @private
+   */
+  tickValuesChanged_: function() {
+    this.$.slider.max = this.tickValues.length - 1;
+    this.valueChanged_();
+  },
+
+  /**
+   * Updates the knob position when |value| changes. If the knob is still being
+   * dragged, this instead forces |value| back to the current position.
+   * @private
+   */
+  valueChanged_: function() {
+    if (typeof this.value == 'undefined' ||
+        typeof this.tickValues == 'undefined') {

[Dan Beam, 2016/05/17 19:35:45]

can't we just change how this observer is defined such that these are both
defined, i.e.

  observers: ['valueChanged_(value, tickValues)'],

or

  value: {
    type: Number,
    notify: true,
    observer: 'valueChanged_(tickValues)',
  },

does that simplify this code a little bit?

[michaelpg, 2016/05/18 17:05:48]

Done.

+      return;
+    }
+
+    if (this.$.slider.dragging &&
+        this.value != this.tickValues[this.$.slider.immediateValue]) {
+      // The pref changed somewhere else (e.g., sync) but we're still holding
+      // the knob, so set the pref back to where we had it.
+      // Async so we don't confuse Polymer's data binding.
+      this.async(function() {
+        this.value = this.tickValues[this.$.slider.immediateValue];
+      });
+      return;
+    }
+
+    // Convert from the public |value| to the slider index (where the knob
+    // should be positioned on the slider).
+    this.$.slider.value = this.findNearestIndex_(this.tickValues, this.value);
+  },
+
+  /**
+   * Returns the index of the item in |arr| closest to |value|. Same cost as
+   * Array.prototype.indexOf if an exact match exists.
+   * @param {!Array<number>} arr
+   * @param {number} value
+   * @return {number}
+   * @private
+   */
+  findNearestIndex_: function(arr, value) {

[Dan Beam, 2016/05/17 19:35:45]

why is findNearestIndex_ a separate method?

[michaelpg, 2016/05/18 17:05:49]

mostly testability and documentability

+    assert(arr.length);
+
+    // The common case has an exact match.
+    var closestIndex = arr.indexOf(value);
+    if (closestIndex != -1)
+      return closestIndex;

[Dan Beam, 2016/05/17 19:35:45]

why do you want to run both indexOf() and the loop below?  isn't the code
duplicative?  it probably doesn't matter for general performance, just wondering
why we need the extra search.

[michaelpg, 2016/05/18 17:05:48]

like the comment says: 99.99% of the time indexOf will work and we'll return, so
why do all the math below?

that said... i think i can address both your comments at the same time, sort of.

+
+    // No exact match. Find the element closest to |value|.
+    var minDifference = Number.MAX_VALUE;
+    for (var i = 0; i < arr.length; i++) {
+      var difference = Math.abs(arr[i] - value);
+      if (difference < minDifference) {
+        closestIndex = i;
+        minDifference = difference;
+      }
+    }
+
+    return closestIndex;
+  },
+});