Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

Add optional numerical input box to dcc.Slider #944

Open
wants to merge 29 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
c858dc9
add input that shows slider value
Feb 25, 2021
6aeea78
sync slider from input
Mar 4, 2021
d57bf70
lint
Mar 4, 2021
7fc4de4
fix lint errors
Mar 4, 2021
e24c3f5
fix lint errors
Mar 4, 2021
796d30d
debounce and components on same line
Mar 6, 2021
c5b7e51
set min width of synced input
Mar 12, 2021
168a99c
pass through props for synced input
Mar 18, 2021
2e2dbe0
let user override default styles for input
Mar 29, 2021
57936d4
run linter
Mar 29, 2021
205b9ed
set proper props for input
Apr 1, 2021
3168a64
refactor input state/props sync
Apr 1, 2021
128017b
update tests
Apr 6, 2021
b555515
Merge branch 'dev' into synced-input-slider
Apr 6, 2021
3e62912
add tests
Apr 6, 2021
3820ce4
lint tests
Apr 6, 2021
884cccb
run prettier
Apr 6, 2021
403ee9e
small fixups
Apr 6, 2021
761103c
make sure that pressing enter when input is focused changes state of …
Apr 8, 2021
1709c21
clear debounce timeout after every state sync
Apr 8, 2021
2db424b
Merge branch 'dev' into synced-input-slider
Apr 13, 2021
da7d116
Merge branch 'synced-input-slider' of https://github.com/plotly/dash-…
Apr 13, 2021
4b45284
refactor synced input event handling
Apr 13, 2021
b7c3154
Merge branch 'dev' into synced-input-slider
Apr 20, 2021
aa1ab08
Merge branch 'dev' into synced-input-slider
Apr 27, 2021
0111a74
camelCase to snake_case new props
Apr 27, 2021
02c10c6
Merge branch 'dev' into synced-input-slider
wbrgss May 14, 2021
aebce8d
add some ui ;ogic
May 21, 2021
f445f58
Merge branch 'synced-input-slider' of https://github.com/plotly/dash-…
May 21, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 33 additions & 1 deletion src/components/Slider.react.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,40 @@ Slider.propTypes = {
}),

/**
* Value by which increments or decrements are made
* Value by which increments or decrements are made.
*/
step: PropTypes.number,

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* 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,

/**
* If true, the slider will be vertical
*/
Expand Down Expand Up @@ -199,6 +229,8 @@ Slider.defaultProps = {
persisted_props: ['value'],
persistence_type: 'local',
verticalHeight: 400,
syncedInputDebounceTime: 450,
syncedInputID: 'syncedInput',
};

export const propTypes = Slider.propTypes;
Expand Down
64 changes: 63 additions & 1 deletion src/fragments/Slider.react.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import React, {Component} from 'react';
import ReactSlider, {createSliderWithTooltip} from 'rc-slider';
import {assoc, omit, pickBy} from 'ramda';
import computeSliderStyle from '../utils/computeSliderStyle';
import Input from '../components/Input.react.js';

import 'rc-slider/assets/index.css';

Expand All @@ -16,8 +17,20 @@ export default class Slider extends Component {
this.DashSlider = props.tooltip
? createSliderWithTooltip(ReactSlider)
: ReactSlider;
this.SyncedInput = Input;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this._computeStyle = computeSliderStyle();
this.state = {value: props.value};
this.syncInput = this.syncInput.bind(this);
}

syncInput(event) {
if (event) {
this.setState({value: Number(event.target.value)});
this.props.setProps({
value: Number(event.target.value),
drag_value: Number(event.target.value),
});
}
}

UNSAFE_componentWillReceiveProps(newProps) {
Expand Down Expand Up @@ -47,6 +60,13 @@ export default class Slider extends Component {
setProps,
tooltip,
updatemode,
syncedInput,
syncedInputDebounceTime,
syncedInputClassName,
syncedInputStyle,
syncedInputID,
style,
step,
vertical,
verticalHeight,
} = this.props;
Expand All @@ -72,15 +92,57 @@ export default class Slider extends Component {
)
: this.props.marks;

const computedStyle = this._computeStyle(
vertical,
verticalHeight,
tooltip
);

const defaultInputStyle = {
width: '60px',
marginRight: vertical && syncedInput ? '' : '25px',
marginBottom: vertical && syncedInput ? '25px' : '',
};

return (
<div
id={id}
data-dash-is-loading={
(loading_state && loading_state.is_loading) || undefined
}
className={className}
style={this._computeStyle(vertical, verticalHeight, tooltip)}
style={{...computedStyle, ...style}}
>
{syncedInput ? (
<this.SyncedInput
onChange={event => {
event.persist();
if (this.timeout) {
clearTimeout(this.timeout);
}
this.timeout = setTimeout(
function() {
this.syncInput(event);
}.bind(this),
syncedInputDebounceTime
);
}}
onBlur={event => {
this.syncInput(event);
}}
onKeyPress={event => {
if (event.key === 'Enter') {
this.syncInput();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}}
type="number"
value={value}
step={step}
className={syncedInputClassName}
id={syncedInputID}
style={{...defaultInputStyle, ...syncedInputStyle}}
/>
) : null}
<this.DashSlider
onChange={value => {
if (updatemode === 'drag') {
Expand Down
3 changes: 3 additions & 0 deletions src/utils/computeSliderStyle.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ export default () => {
return memoizeWith(identity, (vertical, verticalHeight, tooltip) => {
const style = {
padding: '25px',
display: 'flex',
};

if (vertical) {
style.height = verticalHeight + 'px';
style.flexDirection = 'column';

if (
!tooltip ||
Expand All @@ -28,6 +30,7 @@ export default () => {
) {
style.paddingTop = '0px';
}
style.alignItems = 'center';
}

return style;
Expand Down
99 changes: 99 additions & 0 deletions tests/integration/sliders/test_sliders.py
Original file line number Diff line number Diff line change
Expand Up @@ -241,3 +241,102 @@ def update_output(value):
dash_dcc.wait_for_text_to_equal("#out-value", "You have selected 5-15")
dash_dcc.release()
dash_dcc.wait_for_text_to_equal("#out-value", "You have selected 10-15")


def test_slsl008_horizontal_slider_with_input(dash_dcc):
app = dash.Dash(__name__)
app.layout = html.Div(
[
dcc.Slider(
id="slider",
min=0,
max=20,
step=1,
value=5,
syncedInput=True,
syncedInputID="syncedInput",
tooltip={"always_visible": True},
),
html.Div(id="out"),
]
)

@app.callback(Output("out", "children"), [Input("slider", "value")])
def update_output(value):
return "You have selected {}".format(value)

dash_dcc.start_server(app)
dash_dcc.wait_for_text_to_equal("#out", "You have selected 5")

input_ = dash_dcc.find_element("#syncedInput")

input_.clear()
input_.send_keys("8")

dash_dcc.wait_for_text_to_equal("#out", "You have selected 8")


def test_slsl009_vertical_slider_with_input(dash_dcc):
app = dash.Dash(__name__)
app.layout = html.Div(
[
dcc.Slider(
id="slider",
min=0,
max=20,
step=1,
value=5,
vertical=True,
syncedInput=True,
syncedInputID="arbitraryID",
tooltip={"always_visible": True},
),
html.Div(id="out"),
]
)

@app.callback(Output("out", "children"), [Input("slider", "value")])
def update_output(value):
return "You have selected {}".format(value)

dash_dcc.start_server(app)
dash_dcc.wait_for_text_to_equal("#out", "You have selected 5")

input_ = dash_dcc.find_element("#arbitraryID")

input_.clear()
input_.send_keys("8")

dash_dcc.wait_for_text_to_equal("#out", "You have selected 8")


def test_slsl010_horizontal_slider_with_input(dash_dcc):
app = dash.Dash(__name__)
app.layout = html.Div(
[
dcc.Slider(
id="slider",
min=0,
max=20,
step=1,
value=5,
vertical=True,
syncedInput=True,
syncedInputClassName="slider-input",
syncedInputID="arbitraryID",
tooltip={"always_visible": True},
),
html.Div(id="out"),
]
)

@app.callback(Output("out", "children"), [Input("slider", "value")])
def update_output(value):
return "You have selected {}".format(value)

dash_dcc.start_server(app)
dash_dcc.wait_for_text_to_equal("#out", "You have selected 5")

input_ = dash_dcc.find_element("#arbitraryID")

assert input_.get_attribute("class") == "slider-input"
55 changes: 55 additions & 0 deletions tests/test_integration_1.py
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,61 @@ def test_vertical_slider(self):
for entry in self.get_log():
raise Exception("browser error logged during test", entry)

def test_horizontal_slider_with_input(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

app = dash.Dash(__name__)

app.layout = html.Div(
[
html.Label("Horizontal Slider with Input"),
dcc.Slider(
id="horizontal-slider-with-input",
min=0,
max=9,
value=5,
syncedInputClassName="arbitraryClassName",
syncedInput=True,
),
],
style={"height": "500px"},
)
self.startServer(app)

self.wait_for_element_by_css_selector("#horizontal-slider-with-input")
self.wait_for_element_by_css_selector(".arbitraryClassName")

self.snapshot("horizontal slider with input")

for entry in self.get_log():
raise Exception("browser error logged during test", entry)

def test_vertical_slider_with_input(self):
app = dash.Dash(__name__)

app.layout = html.Div(
[
html.Label("Vertical Slider with Input"),
dcc.Slider(
id="vertical-slider-with-input",
min=0,
max=9,
value=5,
vertical=True,
syncedInputClassName="arbitraryClassName",
syncedInput=True,
),
],
style={"height": "500px"},
)
self.startServer(app)

self.wait_for_element_by_css_selector("#vertical-slider-with-input")
self.wait_for_element_by_css_selector(".arbitraryClassName")

self.snapshot("vertical slider with input")

for entry in self.get_log():
raise Exception("browser error logged during test", entry)

def test_loading_range_slider(self):
lock = Lock()

Expand Down