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

Slider tooltips #564

Merged
merged 15 commits into from
Jun 12, 2019
Merged

Slider tooltips #564

merged 15 commits into from
Jun 12, 2019

Conversation

wbrgss
Copy link
Contributor

@wbrgss wbrgss commented Jun 8, 2019

This PR enables tooltips over dcc.Slider and dcc.RangeSlider handles, either on hover or always. Motivated by a customer request via @cldougl, but it's been desired in our open-source community as well.

Adds a new property tooltip that passes in parameters from tipProps, an object native to the rc-slider createSliderWithTooltip API. tipProps isn't very well documented but AFAIK it essentially mirrors the rc-tooltip API. See example of tipProps usage in React here.

@@ -36,7 +36,7 @@
"moment": "^2.20.1",
"prop-types": "^15.6.0",
"ramda": "^0.24.1",
"rc-slider": "^8.3.1",
"rc-slider": "^8.6.11",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Necessary minor upgrade that adds the tipProps API (undocumented; last update to the changelog / HISTORY.MD is from 2016).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like its repo has moved to https://github.com/react-component/slider - HISTORY.MD is still pretty sparse there, but tipProps appears to have been added back in 8.1.something. Anyway, I'm happy to have this up to date!

@@ -152,6 +157,49 @@ RangeSlider.propTypes = {
*/
pushable: PropTypes.oneOfType([PropTypes.bool, PropTypes.number]),

tooltip: PropTypes.oneOfType([
Copy link
Contributor Author

@wbrgss wbrgss Jun 8, 2019

Choose a reason for hiding this comment

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

This is a pattern that @Marc-Andre-Rivet suggested to me. Example usage here in DataTable. However, it's not working for me, allowing e.g.

    dcc.RangeSlider(
        id='my-range-slider',
        min=0,
        max=20,
        step=0.5,
        value=[5,10],
        tooltip={
            'foo': 'bar' # I expect this not to be allowed with the strict type-checking below
        }
    ),

Copy link
Collaborator

Choose a reason for hiding this comment

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

Huh, not sure what's going on there, looks to me like that should work.

Note though that your last one, PropTypes.exact({visible, position}), actually encompasses both of the other two as well as {} since neither is required. Unless you specifically want to prevent tooltip={} from creating a tooltip (and to my mind that seems like a fine way for users to say "give me a default tooltip") I'd just use that last form by itself, no oneOfType.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless you specifically want to prevent tooltip={} from creating a tooltip

IIRC @Marc-Andre-Rivet did want to prevent this, but I could have misunderstood him. Regardless, I'm inclined to agree tooltip={} should be allowed. propTypes.exact seems to actually allow other types as well (not just objects), which is confusing to me, but that means any truthy value will result in a default tooltip.

As an aside, I'd like to be more informed about stricter type-checking with propTypes and the oneOfType / exact pattern — do you understand why the above doesn't work @Marc-Andre-Rivet ?).

@@ -11,6 +11,9 @@ export default class RangeSlider extends Component {
constructor(props) {
super(props);
this.propsToState = this.propsToState.bind(this);
this.DashSlider = props.tooltip
? createSliderWithTooltip(Range)
: Range;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will break if someone starts out with no tooltips and adds them later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK — I added this back to the render method; that should be fine right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, putting this in the render() method will break dragging behavior (unfortunately my screen recorder can't record a cursor; I'm dragging after hovering on the handle):

drag

I'm not sure I actually understand the problem with this in the constructor. If I have a hot-reload app, and I add/remove the tooltip prop in Range/RangeSlider, Dash will handle it fine. The only thing that will be reset is any value that has been changed via user interaction (i.e. dragging/clicking the handle).

This is a simple app I'm using to test
import dash
import dash_core_components as dcc
import dash_html_components as html

app = dash.Dash(__name__)
server = app.server

app.layout = html.Div([
    html.H1("Tooltips"),
    html.Br(),
    html.H1("Slider, always visible"),
    dcc.Slider(
        id='my-slider',
        min=0,
        max=20,
        step=0.5,
        value=10,
        tooltip={'always_visible': True}
    ),

    html.Br(),

    html.H1("RangeSlider, on hover"),
    dcc.RangeSlider(
        id='my-range-slider',
        min=0,
        max=20,
        step=0.5,
        value=[5,10],
        tooltip={'placement': 'bottom'}
    ),
])

if __name__ == '__main__':
  app.run_server(debug=True)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem is if tooltip is the output of a callback

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right, of course. I've added the same conditional to componentWillReceiveProps and tooltip callabacks + normal drag behaviour appear to be working correctly now.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Add a changelog entry, then 💃

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃

* Determines whether tooltips should always be visible
* (as opposed to the default, visible on hover)
*/
visible: PropTypes.bool,
Copy link
Contributor

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

Choose a reason for hiding this comment

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

visible: bool seems like a fuzzy name & type for hover vs. always visible

Copy link
Contributor Author

@wbrgss wbrgss Jun 11, 2019

Choose a reason for hiding this comment

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

Yeah, I agree. I left it like that because that's the API underneath (see "tooltip" "visible"). But considering our discussion on not needing a 1-1 API match for DCC, if you'd rather have an alias i.e. visible = always_visible I'll add it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like always_visible: bool actually - you'd only use it when it's true since false is the default. And even if you did say always_visible: false, it's pretty clear that that's not the same thing as "never visible", which is the implication of visible: false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I've made the change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

LGTM - @Marc-Andre-Rivet are you comfortable with this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup. All good.

* top/bottom{*} sets the _origin_ of the tooltip, so e.g. `topLeft` will
* in reality appear to be on the top right of the handle
*/
placement: PropTypes.oneOf([
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 made a mistake before setting the key as position, it should be placement

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants