Add optional numerical input box to dcc.Slider

[jdamiba]

partially addresses https://github.com/plotly/dash-core/issues/146

The purpose of this PR is to add an optional numerical input box to the dcc.Slider component. For a demo of what this new feature looks like, see https://core-dev.plotly.host/slider-synced-input/.

Usage: When declaring a dcc.Slider, set syncedInput=True. Moving the slider updates the value in the input, and changing the value in the input updates the slider, after a brief (and editable) debounce delay. The input can have CSS classes and styling applied to it separately from the slider. This PR also introduces a style prop to the dcc.Slider itself, for the sake of feature parity with other DCC components which allow a style prop.

New attributes of the dcc.Slider:

/**
* If true, display an Input component whose value is synced with the Slider's value.
*/
syncedInput: PropTypes.bool,

/**
* The classname to be given to the synced Input component.
*/
syncedInputClassName: PropTypes.string,

/**
* The CSS to be applied to the class of the input (div)
*/
syncedInputStyle: PropTypes.object,

/**
* The id to be applied to the input (div). Default is "syncedInput".
*/
 syncedInputID: PropTypes.string,

/**
 * The CSS to be applied to the class of the slider (div)
 */
style: PropTypes.object,

/**
 * The amount of time the synced Input should wait before passing along state changes without a change of focus or the user pressing Enter. In milliseconds.
 */
syncedInputDebounceTime: PropTypes.number,

To help surface future regressions of this functionality, tests have been added in tests/test_integration_1.py and tests/integration/sliders/test_sliders.py. These tests ensure that (1) changing the value of the input is reflected in the value of the slider, (2) the dcc.Slider does not throw a console error when the synced input is enabled, (3) arbitrary CSS classes and styling is passed through to the input, and (4) Percy screenshots. These tests cover both vertical and horizontal sliders with inputs.

Note: In future PR, this functionality will be extended to the dcc.RangeSlider component.

[jdamiba]

@alexcjohnson Can you please take a look at this PR? I think it's ready for review.

[alexcjohnson]

Probably don't need this indirection - this.DashSlider is abstracted because there are two different versions of the component depending on whether tooltips are requested (though it occurs to me, the way this is currently implemented it looks like there may be a bug if you add or remove props.tooltip dynamically?), but because Input only has one flavor you should be able to use it directly in the render method.

[alexcjohnson]

Actually, looking at this a little more closely, what's the benefit of using our Input component instead of an HTML input tag directly? The Input is kind of confusing used this way, and if your used input you could attach a ref to it and avoid having to pass event around just to get the value out. (You can attach a ref to a react class as well, but I think that would make it even more confusing to get the value out, because this doesn't make its way back into state or props given the current usage AFAICT...)

[jdamiba]

I agree, in hindsight it was confusing to re-use the dcc.Input component in this context. Instead, I have followed your suggestion and used a ref to a JSX-generated input HTML tag.

[alexcjohnson]

We should probably clear the timeout here and in onBlur (or just inside syncInput?) so we don't call syncInput twice in these cases.

[alexcjohnson]

wait, this particular call has no event so does that mean syncInput won't do anything?

[jdamiba]

This comment is was correct, but has been made outdated by refactoring this component to avoid needing to pass an event around at all. I did take your suggestion and clear the timeout in the syncInputWithSlider function to ensure it is always cleared no matter which event handler is invoked.

[alexcjohnson]

This looks like a great test, but let's not add any more tests to the top-level files test_integration_1 or test_integration_2. These use the legacy unittest framework rather than dash.testing. If anything we should take opportunities when we're working in neighboring code to move more tests OUT of these files and port them to use dash.testing in the subdirectories.

[alexcjohnson]

@jdamiba looking good! Playing with this using pytest -k slsl008 --pause - a little bit of behavior we might want to tweak: It's possible to delete the text in the input box (in which case it gets treated as zero)

or enter a value that's incompatible with the step

Also: if you enter an out-of-range number, after the timeout time it will be replaced by the closest limit value. This is a problem if the limit is bigger than zero, it can prevent you from typing in the number you want. For example set min=10, max=20 and try to type 13 - if you're quick you can get it in various ways, but if you're slower than the timeout you'll end up with 10, then you might type the 3 making 103 which gets pushed to 20.

In all of these cases I think we should allow the value to stay in the input box until blur, and NOT update the slider or the prop. Then on blur my gut reaction is:

• if you cleared the value, reset to the previous prop value. There's also a case where you type a partial number that's not a number, for example "2e" is allowed because "2e3" etc is valid. Also "." - seems like all of these should be treated the same way, reset to the previous prop value.
• if you're out of range, push to the closest limit
• if you're between steps, round to the nearest step

Finally, the reason I commented on this particular line: you're casting to number here, but you already used it above as though it was a number, when it was still a string.

[alexcjohnson]

Per the discussion @chriddyp and I had just now and summarized on Slack: let's convert all these new props to snake_case. In the short term this will make slider a little funny since it already has some camelCase props. That's OK, we'll get to it soon, and adding the backward-compatible conversion of other props is going to take some more work, especially since we use omit in this component.