Skip to content
This repository was archived by the owner on Aug 29, 2025. It is now read-only.

Issue 485 - Slider className / external styling regression #486

Merged
merged 17 commits into from
Mar 18, 2019
Merged

Issue 485 - Slider className / external styling regression #486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
fba21be
Apply class to slider wrapper
Mar 11, 2019
99cf4ba
Merge remote-tracking branch 'origin/master' into issue485-slider-ext…
Mar 11, 2019
cba39b4
update changelog
Mar 11, 2019
b7a21b5
fix lint
Mar 11, 2019
0bd5a0b
Merge branch 'master' into issue485-slider-external-styling
Marc-Andre-Rivet Mar 18, 2019
f2e996a
- range slider loading state
Mar 18, 2019
4d2b99c
lint
Mar 18, 2019
69a1ce8
fix test
Mar 18, 2019
694d140
loading slider test
Mar 18, 2019
1f0bf1a
fix dash code for tests
Mar 18, 2019
4fd527e
verticals
Mar 18, 2019
d681399
wait instead of find
Mar 18, 2019
a5372d4
lint
Mar 18, 2019
e271919
Merge branch 'master' into issue485-slider-external-styling
Marc-Andre-Rivet Mar 18, 2019
70e1eb8
Merge branch 'master' into issue485-slider-external-styling
Marc-Andre-Rivet Mar 18, 2019
738527a
use lock instead of time sleep
Mar 18, 2019
1e4e7e7
Merge branch 'issue485-slider-external-styling' of github.com:plotly/…
Mar 18, 2019
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).

### Fixed
- Fix Vertical Slider regression [#479](https://github.com/plotly/dash/issues/479)
- Fix Slider regression [#485](https://github.com/plotly/dash/issues/485)

## [0.44.0] - 2019-03-04
### Added
Expand Down
52 changes: 34 additions & 18 deletions src/components/RangeSlider.react.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,31 +18,47 @@ export default class RangeSlider extends Component {
}

render() {
const {setProps, updatemode, loading_state} = this.props;
const {
className,
id,
loading_state,
setProps,
updatemode,
vertical,
} = this.props;
const {value} = this.state;
return (
<Range
<div
id={id}
data-dash-is-loading={
(loading_state && loading_state.is_loading) || undefined
}
onChange={value => {
this.setState({value});
if (updatemode === 'drag') {
if (setProps) {
setProps({value});
className={className}
style={vertical ? {height: '100%'} : {}}
>
<Range
onChange={value => {
this.setState({value});
if (updatemode === 'drag') {
if (setProps) {
setProps({value});
}
}
}
}}
onAfterChange={value => {
if (updatemode === 'mouseup') {
if (setProps) {
setProps({value});
}}
onAfterChange={value => {
if (updatemode === 'mouseup') {
if (setProps) {
setProps({value});
}
}
}
}}
value={value}
{...omit(['value', 'setProps', 'updatemode'], this.props)}
/>
}}
value={value}
{...omit(
['className', 'value', 'setProps', 'updatemode'],
this.props
)}
/>
</div>
);
}
}
Expand Down
15 changes: 13 additions & 2 deletions src/components/Slider.react.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,22 @@ export default class Slider extends Component {
}

render() {
const {id, loading_state, setProps, updatemode, vertical} = this.props;
const {
className,
id,
loading_state,
setProps,
updatemode,
vertical,
} = this.props;
const {value} = this.state;
return (
<div
id={id}
data-dash-is-loading={
(loading_state && loading_state.is_loading) || undefined
}
className={className}
Copy link
Contributor Author

@Marc-Andre-Rivet Marc-Andre-Rivet Mar 11, 2019

Choose a reason for hiding this comment

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

Apply class name to the wrapper. rc-slider applies very little styles to itself and relevant styles will be ok if applied to the parent (e.g. it doesn't explicitly set the color, font-size, etc.) unlike the dropdown.

Copy link
Collaborator

@alexcjohnson alexcjohnson Mar 11, 2019

Choose a reason for hiding this comment

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

Most of the other cases where we allow className, we also allow style - do we want to do that here?

Copy link
Contributor Author

@Marc-Andre-Rivet Marc-Andre-Rivet Mar 11, 2019

Choose a reason for hiding this comment

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

No. This is because the wrapper was added with Loading Components and the wrapped slider did not support the style property. I don't mind adding support for styling here -- but will need to handle the existing #481 style applied on the vertical slider.

Copy link
Contributor Author

@Marc-Andre-Rivet Marc-Andre-Rivet Mar 11, 2019

Choose a reason for hiding this comment

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

Fwiw, the rangeslider does not support style either.

Copy link
Collaborator

@alexcjohnson alexcjohnson Mar 11, 2019

Choose a reason for hiding this comment

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

Given that the main purpose of className is styling, it seems appropriate to add style. But as that's a feature rather than a bugfix, feel free to make an issue and defer for later if you don't want to do it in this PR.

style={vertical ? {height: '100%'} : {}}
>
<ReactSlider
Expand All @@ -45,7 +53,10 @@ export default class Slider extends Component {
}
}}
value={value}
{...omit(['setProps', 'updatemode', 'value'], this.props)}
{...omit(
['className', 'setProps', 'updatemode', 'value'],
this.props
)}
/>
</div>
);
Expand Down
175 changes: 175 additions & 0 deletions test/test_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import io
import os
import sys
from multiprocessing import Lock
import time
import json

Expand Down Expand Up @@ -215,6 +216,74 @@ def test_upload_gallery(self):

self.snapshot('test_upload_gallery')

def test_loading_slider(self):
lock = Lock()
lock.acquire()

app = dash.Dash(__name__)

app.layout = html.Div([
html.Label(id='test-div', children=['Horizontal Slider']),
dcc.Slider(
id='horizontal-slider',
min=0,
max=9,
marks={i: 'Label {}'.format(i) if i == 1 else str(i)
for i in range(1, 6)},
value=5,
),
])

@app.callback(
Output('horizontal-slider', 'value'),
[Input('test-div', 'children')]
)
def delayed_value(children):
lock.acquire()
lock.release()
Copy link
Collaborator

@alexcjohnson alexcjohnson Mar 18, 2019

Choose a reason for hiding this comment

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

Very nice @Marc-Andre-Rivet !

@plotly/dash whenever you are tempted to add a time.sleep in tests, take a moment to think "is there a way to do it without a sleep?" A huge fraction of our test time is simply sleep statements, which are set super long in order to be robust.

A lot of the time we can solve this just by waiting for some condition (an element with the right contents, for example). This one is a bit trickier, we were using a sleep in the server process (where callbacks run) to ensure something had time to be observed in the browser-control process, but this commit shows a fairly simple pattern to do this with a Lock so that the server process stops waiting as soon as the browser has seen what it's looking for.

return 5

self.startServer(app)

self.wait_for_element_by_css_selector(
'#horizontal-slider[data-dash-is-loading="true"]'
)
lock.release()

self.wait_for_element_by_css_selector(
'#horizontal-slider:not([data-dash-is-loading="true"])'
)

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

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

app.layout = html.Div([
html.Label('Horizontal Slider'),
dcc.Slider(
id='horizontal-slider',
min=0,
max=9,
marks={i: 'Label {}'.format(i) if i == 1 else str(i)
for i in range(1, 6)},
value=5,
),
])
self.startServer(app)

self.wait_for_element_by_css_selector('#horizontal-slider')
self.snapshot('horizontal slider')

h_slider = self.driver.find_element_by_css_selector(
'#horizontal-slider div[role="slider"]'
)
h_slider.click()

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

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

Expand Down Expand Up @@ -243,6 +312,112 @@ def test_vertical_slider(self):
for entry in self.get_log():
raise Exception('browser error logged during test', entry)

def test_loading_range_slider(self):
Copy link
Contributor Author

@Marc-Andre-Rivet Marc-Andre-Rivet Mar 18, 2019

Choose a reason for hiding this comment

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

For both slider and range slider, make sure that the horizontal / vertical version displays correctly using the previously used styling, that the slider nubs can be clicked without error and that loading state is applied correctly to the DOM element

lock = Lock()
lock.acquire()

app = dash.Dash(__name__)

app.layout = html.Div([
html.Label(id='test-div', children=['Horizontal Range Slider']),
dcc.RangeSlider(
id='horizontal-range-slider',
min=0,
max=9,
marks={i: 'Label {}'.format(i) if i == 1 else str(i)
for i in range(1, 6)},
value=[4, 6],
),
])

@app.callback(
Output('horizontal-range-slider', 'value'),
[Input('test-div', 'children')]
)
def delayed_value(children):
lock.acquire()
lock.release()
return [4, 6]

self.startServer(app)

self.wait_for_element_by_css_selector(
'#horizontal-range-slider[data-dash-is-loading="true"]'
)
lock.release()

self.wait_for_element_by_css_selector(
'#horizontal-range-slider:not([data-dash-is-loading="true"])'
)

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

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

app.layout = html.Div([
html.Label('Horizontal Range Slider'),
dcc.RangeSlider(
id='horizontal-range-slider',
min=0,
max=9,
marks={i: 'Label {}'.format(i) if i == 1 else str(i)
for i in range(1, 6)},
value=[4, 6],
),
])
self.startServer(app)

self.wait_for_element_by_css_selector('#horizontal-range-slider')
self.snapshot('horizontal range slider')

h_slider_1 = self.driver.find_element_by_css_selector(
'#horizontal-range-slider div.rc-slider-handle-1[role="slider"]'
)
h_slider_1.click()

h_slider_2 = self.driver.find_element_by_css_selector(
'#horizontal-range-slider div.rc-slider-handle-2[role="slider"]'
)
h_slider_2.click()

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

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

app.layout = html.Div([
html.Label('Vertical Range Slider'),
dcc.RangeSlider(
id='vertical-range-slider',
min=0,
max=9,
marks={i: 'Label {}'.format(i) if i == 1 else str(i)
for i in range(1, 6)},
value=[4, 6],
vertical=True,
),
], style={'height': '500px'})
self.startServer(app)

self.wait_for_element_by_css_selector('#vertical-range-slider')
self.snapshot('vertical range slider')

v_slider_1 = self.driver.find_element_by_css_selector(
'#vertical-range-slider div.rc-slider-handle-1[role="slider"]'
)
v_slider_1.click()

v_slider_2 = self.driver.find_element_by_css_selector(
'#vertical-range-slider div.rc-slider-handle-2[role="slider"]'
)
v_slider_2.click()

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

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

Expand Down