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

Omit marks that are outside of range specified by min and max. #695

Merged
merged 27 commits into from
Nov 19, 2019
Merged

Omit marks that are outside of range specified by min and max. #695

Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
709f46c
Omit marks that are outside of range specified by min and max.
Nov 7, 2019
3bf1241
Merge branch 'dev' into 517-rangeslider-labels
Nov 7, 2019
480511f
Handle case in which marks prop is not defined.
Nov 7, 2019
01324bf
Add test for out-of-range numbers.
Nov 8, 2019
5cb8509
Use pickBy.
Nov 11, 2019
9c94308
Add mark at point below minimum value.
Nov 11, 2019
396756e
Also omit out-of-range marks for slider.
Nov 11, 2019
4bdaf71
Add test for slider.
Nov 11, 2019
1f5d15d
Add padding to Slider and RangeSlider containers.
Nov 12, 2019
a93fa42
Update test for persistence.
Nov 12, 2019
fdd296b
Change test for always visible rangeslider.
Nov 13, 2019
ad838fc
Only add top padding if there are always-visible tooltips on the top.
Nov 14, 2019
c8070d4
Preserve whitespace in marks.
Nov 14, 2019
9e2ee6b
Add optional verticalHeight prop for vertical sliders.
Nov 14, 2019
4d03221
Update slider stylesheet.
Nov 14, 2019
5a6a845
Merge branch 'dev' into 517-rangeslider-labels
Nov 14, 2019
bbb4fea
Update coordinates to reflect new padding.
Nov 14, 2019
a0aad56
Remove file.
Nov 14, 2019
1261f90
Use fixed-width slider for rangeslider test.
Nov 14, 2019
e4476ea
Merge branch 'dev' into 517-rangeslider-labels
Nov 14, 2019
b7dee1a
Fix persistence test.
Nov 14, 2019
edc2784
Memoize computation of style and move function to utils.
Nov 19, 2019
df14f7d
Simplify style code.
Nov 19, 2019
b9a8fcf
Fix eslint errors.
Nov 19, 2019
dfee39a
Merge branch 'dev' into 517-rangeslider-labels
Nov 19, 2019
3e32f28
Modify style object directly.
Nov 19, 2019
473e86a
Update CHANGELOG.
Nov 19, 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
58 changes: 55 additions & 3 deletions src/components/RangeSlider.react.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React, {Component} from 'react';
import PropTypes from 'prop-types';
import {assoc, omit} from 'ramda';
import {assoc, omit, contains, pickBy, merge} from 'ramda';
import {Range, createSliderWithTooltip} from 'rc-slider';

/**
Expand Down Expand Up @@ -42,6 +42,7 @@ export default class RangeSlider extends Component {
tooltip,
updatemode,
vertical,
verticalHeight,
} = this.props;
const value = this.state.value;

Expand All @@ -58,14 +59,51 @@ export default class RangeSlider extends Component {
tipProps = tooltip;
}

const truncatedMarks = this.props.marks
? pickBy(
(k, mark) => mark >= this.props.min && mark <= this.props.max,
this.props.marks
)
: this.props.marks;
Copy link
Contributor

Choose a reason for hiding this comment

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

A matter of personal preference, but you can also make this more compact with

const truncatedMarks = this.props.marks && pickBy(...);

JS does not return a boolean for &&, ||, etc. but rather the last evaluated value.


let style = {
padding: '25px',
};

if (
!vertical &&
(!tooltip ||
!tooltip.always_visible ||
!contains(tooltip.placement, ['top', 'topLeft', 'topRight']))
) {
style = merge(style, {paddingTop: '0px'});
}

if (
vertical &&
(!tooltip ||
!tooltip.always_visible ||
!contains(tooltip.placement, [
'left',
'topRight',
'bottomRight',
]))
) {
style = merge(style, {paddingLeft: '0px'});
}

if (vertical) {
style = merge(style, {height: verticalHeight + 'px'});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Could be simplified with shared vertical/!vertical checks.
  • Minor point, could be memoized (style, vertical, tooltip) -> style to prevent style re-evaluation below


return (
<div
id={id}
data-dash-is-loading={
(loading_state && loading_state.is_loading) || undefined
}
className={className}
style={vertical ? {height: '100%'} : {}}
style={style}
>
<this.DashSlider
onChange={value => {
Expand All @@ -82,8 +120,16 @@ export default class RangeSlider extends Component {
}}
tipProps={tipProps}
value={value}
marks={truncatedMarks}
{...omit(
['className', 'value', 'setProps', 'updatemode'],
[
'className',
'value',
'setProps',
'marks',
'updatemode',
'verticalHeight',
],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not for now, but just because I'm thinking about it: I'd like us to consider moving away from omit for forwarding props to 3rd-party components, and use pick instead. IMO omit makes it too hard to follow which props are being passed on, too easy to forget to remove newly added props, and makes us lazy about accepting someone else's choice of names and values even if they don't make sense in our case. (cc @Marc-Andre-Rivet )

Copy link
Contributor

Choose a reason for hiding this comment

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

@alexcjohnson Agreed for DCC! There's also a bit of this happening in the table fragments / d3-format wrappers that should be looked into. Due to its generated nature I'd say whether this makes sense on HTML components would have to be evaluated independently. Can you open an issue for follow up and tag it for Dash v1.x milestone? Thanks.

this.props
)}
/>
Expand Down Expand Up @@ -213,6 +259,11 @@ RangeSlider.propTypes = {
*/
vertical: PropTypes.bool,

/**
* The height, in px, of the slider if it is vertical.
*/
verticalHeight: PropTypes.number,

/**
* Determines when the component should update
* its value. If `mouseup`, then the slider
Expand Down Expand Up @@ -281,4 +332,5 @@ RangeSlider.defaultProps = {
updatemode: 'mouseup',
persisted_props: ['value'],
persistence_type: 'local',
verticalHeight: 400,
};
58 changes: 55 additions & 3 deletions src/components/Slider.react.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import React, {Component} from 'react';
import ReactSlider, {createSliderWithTooltip} from 'rc-slider';
import PropTypes from 'prop-types';
import {assoc, omit} from 'ramda';
import {assoc, omit, pickBy, contains, merge} from 'ramda';
import './css/[email protected]';

/**
Expand Down Expand Up @@ -42,6 +42,7 @@ export default class Slider extends Component {
tooltip,
updatemode,
vertical,
verticalHeight,
} = this.props;
const value = this.state.value;

Expand All @@ -58,14 +59,51 @@ export default class Slider extends Component {
tipProps = tooltip;
}

const truncatedMarks = this.props.marks
? pickBy(
(k, mark) => mark >= this.props.min && mark <= this.props.max,
this.props.marks
)
: this.props.marks;

let style = {
padding: '25px',
};

if (
!vertical &&
(!tooltip ||
!tooltip.always_visible ||
!contains(tooltip.placement, ['top', 'topLeft', 'topRight']))
) {
style = merge(style, {paddingTop: '0px'});
}

if (
vertical &&
(!tooltip ||
!tooltip.always_visible ||
!contains(tooltip.placement, [
'left',
'topRight',
'bottomRight',
]))
) {
style = merge(style, {paddingLeft: '0px'});
}

if (vertical) {
style = merge(style, {height: verticalHeight + 'px'});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above


return (
<div
id={id}
data-dash-is-loading={
(loading_state && loading_state.is_loading) || undefined
}
className={className}
style={vertical ? {height: '100%'} : {}}
style={style}
>
<this.DashSlider
onChange={value => {
Expand All @@ -82,8 +120,16 @@ export default class Slider extends Component {
}}
tipProps={tipProps}
value={value}
marks={truncatedMarks}
{...omit(
['className', 'setProps', 'updatemode', 'value'],
[
'className',
'setProps',
'updatemode',
'value',
'marks',
'verticalHeight',
],
this.props
)}
/>
Expand Down Expand Up @@ -194,6 +240,11 @@ Slider.propTypes = {
*/
vertical: PropTypes.bool,

/**
* The height, in px, of the slider if it is vertical.
*/
verticalHeight: PropTypes.number,

/**
* Determines when the component should update
* its value. If `mouseup`, then the slider
Expand Down Expand Up @@ -262,4 +313,5 @@ Slider.defaultProps = {
updatemode: 'mouseup',
persisted_props: ['value'],
persistence_type: 'local',
verticalHeight: 400,
};
2 changes: 2 additions & 0 deletions src/components/css/[email protected]
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@
text-align: center;
cursor: pointer;
color: #999;
white-space: pre;
width: fit-content;
}
.rc-slider-mark-text-active {
color: #666;
Expand Down
8 changes: 4 additions & 4 deletions tests/integration/misc/test_persistence.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,11 +147,11 @@ def make_output(*args):
dash_dcc.find_element("#radioitems label:first-child input").click() # red

range_slider = dash_dcc.find_element("#rangeslider")
dash_dcc.click_at_coord_fractions(range_slider, 0.01, 0.5) # 0
dash_dcc.click_at_coord_fractions(range_slider, 0.5, 0.5) # 5
dash_dcc.click_at_coord_fractions(range_slider, 0.5, 0.25) # 5
dash_dcc.click_at_coord_fractions(range_slider, 0.8, 0.25) # 8

slider = dash_dcc.find_element("#slider")
dash_dcc.click_at_coord_fractions(slider, 0.2, 0.5) # 22
dash_dcc.click_at_coord_fractions(slider, 0.2, 0.25) # 22

dash_dcc.find_element("#tabs .tab:last-child").click() # C

Expand All @@ -166,7 +166,7 @@ def make_output(*args):
[u"4️⃣", u"6️⃣"],
"yes maybe",
"r",
[0, 5],
[5, 8],
22,
"C",
"knock knock\nwho's there?",
Expand Down
42 changes: 37 additions & 5 deletions tests/integration/sliders/test_sliders.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,15 @@ def update_output(value):
dash_dcc.wait_for_text_to_equal("#out", "You have selected 5")

slider = dash_dcc.find_element("#slider")
dash_dcc.click_at_coord_fractions(slider, 0.5, 0.5)
dash_dcc.click_at_coord_fractions(slider, 0.5, 0.25)
dash_dcc.wait_for_text_to_equal("#out", "You have selected 10")
dash_dcc.click_at_coord_fractions(slider, 0.75, 0.5)
dash_dcc.click_at_coord_fractions(slider, 0.75, 0.25)
dash_dcc.wait_for_text_to_equal("#out", "You have selected 15")


def test_slsl002_always_visible_rangeslider(dash_dcc):
app = dash.Dash(__name__)
app.layout = html.Div([
app.layout = html.Div(style={'width': '400px'}, children=[
dcc.RangeSlider(
id="rangeslider",
min=0,
Expand All @@ -54,7 +54,39 @@ def update_output(rng):
dash_dcc.wait_for_text_to_equal("#out", "You have selected 5-15")

slider = dash_dcc.find_element("#rangeslider")
dash_dcc.click_at_coord_fractions(slider, 0.1, 0.5)
dash_dcc.click_at_coord_fractions(slider, 0.15, 0.25)
dash_dcc.wait_for_text_to_equal("#out", "You have selected 2-15")
dash_dcc.click_at_coord_fractions(slider, 0.5, 0.5)
dash_dcc.click_at_coord_fractions(slider, 0.5, 0.25)
dash_dcc.wait_for_text_to_equal("#out", "You have selected 2-10")


def test_slsl003_out_of_range_marks_slider(dash_dcc):

app = dash.Dash(__name__)
app.layout = html.Div([
dcc.Slider(
min=0,
max=5,
marks={i: 'Label {}'.format(i) for i in range(-1, 10)}
)
])

dash_dcc.start_server(app)

assert len(dash_dcc.find_elements('span.rc-slider-mark-text')) == 6


def test_slsl004_out_of_range_marks_rangeslider(dash_dcc):

app = dash.Dash(__name__)
app.layout = html.Div([
dcc.RangeSlider(
min=0,
max=5,
marks={i: 'Label {}'.format(i) for i in range(-1, 10)}
)
])

dash_dcc.start_server(app)

assert len(dash_dcc.find_elements('span.rc-slider-mark-text')) == 6