Issue 485 - Slider className / external styling regression
#486
Conversation
…ernal-styling # Conflicts: # src/components/Slider.react.js
| const {value} = this.state; | ||
| return ( | ||
| <div | ||
| id={id} | ||
| data-dash-is-loading={ | ||
| (loading_state && loading_state.is_loading) || undefined | ||
| } | ||
| className={className} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Most of the other cases where we allow className, we also allow style - do we want to do that here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Fwiw, the rangeslider does not support style either.
There was a problem hiding this comment.
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.
|
Can we 🔒 this with a test? Does this fix (and #479 too I guess) need to be applied to |
|
This does not affect the range slider, as for some reason that slider (same 3rd party package) supported data-* props |
|
OK, I don't see what |
|
data-* lack of support was what caused the slider to be wrapped in the first place for the LC feature. Yeah, writing some additional tests. |
|
So.. digging a little bit more in all directions, turns out the range slider should have been wrapped too. I was assuming it worked here #486 (comment) as the difference sort of assumed that in my mind.. anyway, LC attributes don't work there either. Not sure why the slider was tested/wrapped and not the range slider. This is the only 3rd party element that isn't wrapped, not sure how we came to the conclusion it was behaving correctly with LC.. |
- additional visual tests for range slider
| @@ -245,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): | |||
There was a problem hiding this comment.
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
…dash-core-components into issue485-slider-external-styling
| @@ -237,14 +241,16 @@ def test_loading_slider(self): | |||
| [Input('test-div', 'children')] | |||
| ) | |||
| def delayed_value(children): | |||
| time.sleep(5) | |||
| lock.acquire() | |||
| lock.release() | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
💃
Please make an issue for a slider style prop but doesn't need to be done here #486 (comment)
Fixes #485