-
-
Notifications
You must be signed in to change notification settings - Fork 146
Omit marks that are outside of range specified by min and max. #695
Conversation
@alexcjohnson Right now I'm just omitting all labels that fall outside of the range specified for the slider, but I also agree with this comment you made:
Do you feel it would be a good idea to include some new property to "disable" the options for the ones outside of a specific range (but still show them)? |
No, I think what you did here is exactly what we want, no new options needed 🎉 |
'setProps', | ||
'marks', | ||
'updatemode', | ||
], |
There was a problem hiding this comment.
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 )
There was a problem hiding this comment.
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.
Two more questions:
|
6540ebb
to
6a69f15
Compare
47d701d
to
9c94308
Compare
Yes, this makes sense.
I'll take a look at the CSS! |
With the new padding values, the '0' selection is no longer at the very edge of the container div.
@alexcjohnson Made some changes to the padding and now it looks good! |
Some other changes made (cc @wbrgss since this affects how the slider CSS looks -- would love any suggestions you have):
|
@shammamah These look like good changes to me. In DDK, we're "documenting around" (private docs, see "dcc.Slider best practices" section) some of these issues, including vertical height:
This seems like a good idea to me, and so does
I think that will be adequate to prevent this situation: |
@shammamah What about labels that overlap horizontally, because there are too many of them to fit?
Oh sorry, didn't see this. I think that's good for now 👍 |
src/components/RangeSlider.react.js
Outdated
(k, mark) => mark >= this.props.min && mark <= this.props.max, | ||
this.props.marks | ||
) | ||
: this.props.marks; |
There was a problem hiding this comment.
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.
src/components/RangeSlider.react.js
Outdated
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'}); | ||
} |
There was a problem hiding this comment.
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
src/components/Slider.react.js
Outdated
|
||
if (vertical) { | ||
style = merge(style, {height: verticalHeight + 'px'}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above
src/utils/computeSliderStyle.js
Outdated
}; | ||
|
||
if (vertical) { | ||
style = merge(style, {height: verticalHeight + 'px'}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non blocking -- style
is created inside the function scope, it's fine to consider it as mutable inside the function - as long as it's treated as immutable once outside
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💃 🎚
* Omit marks that are outside of range specified by min and max. * Handle case in which marks prop is not defined. * Add test for out-of-range numbers. * Use pickBy. * Add mark at point below minimum value. * Also omit out-of-range marks for slider. * Add test for slider. * Add padding to Slider and RangeSlider containers. * Update test for persistence. With the new padding values, the '0' selection is no longer at the very edge of the container div. * Change test for always visible rangeslider. * Only add top padding if there are always-visible tooltips on the top. * Preserve whitespace in marks. * Add optional verticalHeight prop for vertical sliders. * Update slider stylesheet. * Update coordinates to reflect new padding. * Remove file. * Use fixed-width slider for rangeslider test. * Fix persistence test. * Memoize computation of style and move function to utils. * Simplify style code. * Fix eslint errors. * Modify style object directly. * Update CHANGELOG.
Closes #517.