Feature: Selection Modes

[musicist288]

This is a proposed implementation for #371.

Selection modes are defined in src/dygraph-selection-modes.js with the
ability to override them on a per graph basis with the option
selectionMode when instantiating the Dygraph.

This is a generalized approach of the changes I needed to make for my company's use case. These changes represent the bare minimum; I wasn't sure if I should go as far as to define a whole selection model implementation (similar to the plug-in system). Our use case revolves exclusively around time series data so there may be some decisions made that are influenced by that case and may not fit the general approach.

[danvk]

Thanks for this change! I'll make an effort to review it this week.

[danvk]

I left a few smaller comments, but overall I'm not sure I understand what's going on. I thought the idea of this change was to support both "select by closest x-distance" and "select by closest euclidean distance," but the latter doesn't seem to be a part of this change. What's the difference between selectByRow and selectByClosestX?

---

Review status: 0 of 5 files reviewed at latest revision, 7 unresolved discussions.

---

src/dygraph-options-reference.js, line 854 [r1] (raw file):
closest (sp)

---

src/dygraph-options-reference.js, line 855 [r1] (raw file):
I'd like to move away from passing the dygraph as a positional parameter to callbacks. It should be accessed as this.

---

src/dygraph-options-reference.js, line 858 [r1] (raw file):
It's → Its

---

src/dygraph-selection-modes.js, line 8 [r1] (raw file):
fileoverview

---

src/dygraph-selection-modes.js, line 10 [r1] (raw file):
dygraph.js

---

src/dygraph-selection-modes.js, line 17 [r1] (raw file):
"that strictly" → did a word get dropped here?

---

src/dygraph.js, line 3578 [r1] (raw file):
any reason not to just do:

Dygraph.SelectionModes = DygraphSelectionModes;

?

---

Comments from the review on Reviewable.io

[musicist288]

The difference is that with selectByRow, if the selected row does not have a value for a series, the selection will not include a value for that series. selectByClosestX starts at the current row, but if no value is found for a series, it looks for the last defined value for that series in row numbers less than the selected row. Perhaps a more appropriate name would be something like "selectByLatestDefinedValue"?

Perhaps I let my needs dilute my understanding of the original feature request I'm exclusively visualizing time-series data. One chart type in particular has data where a single row would almost never have defined values for every series. My goal was to get the highlight dots and the legend reporting the last value reported for the series up until the current time, but never after, and have the legend report the time value of the latest data point.

Here's a GIF of the interaction I needed. This is the most extreme case, but I have other examples if needed.

I'm not too concerned about this specific selection model making it into the library if we can't agree about the semantics of the selection mode. I'm mostly concerned about the callback option being exposed.

---

Review status: 0 of 5 files reviewed at latest revision, 3 unresolved discussions.

---

src/dygraph-options-reference.js, line 855 [r1] (raw file):
Ok. I wasn't sure about that. I'll change it.

---

src/dygraph-selection-modes.js, line 17 [r1] (raw file):
Not dropped, but I was editing that block a lot. I'll re-word it.

---

src/dygraph.js, line 3578 [r1] (raw file):
Nope, not sure why I originally had this. I'll change it to an assignment.

---

Comments from the review on Reviewable.io

[danvk]

I'd accept a version of this if I saw a demonstration that it could be used to fix #371, i.e. support selecting the closest point by euclidean distance. Ideally that would share code with the existing mechanism via highlightSeriesOpts.

[musicist288]

Ok. I'll see if I can do that. I'm, probably just going to maintain a
separate fork for now

On Thu, Dec 3, 2015 at 2:17 PM Dan Vanderkam notifications@github.com
wrote:

I'd accept a version of this if I saw a demonstration that it could be
used to fix #371 #371, i.e.
support selecting the closest point by euclidean distance. Ideally that
would share code with the existing mechanism via highlightSeriesOpts.

—
Reply to this email directly or view it on GitHub
#696 (comment).

[musicist288]

Closing for now. Will reopen if a fix for #371 is implemented.