Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dragging behaviour properties to sliders #2834

Merged
merged 5 commits into from
Sep 17, 2021

Conversation

ibdafna
Copy link
Member

@ibdafna ibdafna commented Mar 24, 2020

This PR adds functionality requested in #2832 . The drag behaviour API has been exposed on the python side and now allows four selections:

  1. Tap (current default behaviour)
  2. Snap (new option)
  3. Drag-tap (new default behaviour for all sliders - no change in behaviour for non-range sliders)
  4. Drag-snap

There are no breaking API changes, but a new behaviour attribute has been added to the python side.

devw_drag_behaviour1

@ibdafna ibdafna requested a review from jasongrout March 24, 2020 05:35
@jasongrout
Copy link
Member

jasongrout commented Mar 24, 2020

This PR adds functionality requested in #2683 .

#2832, right?

I was thinking it might be better to start out with just changing the default behavior to 'drag-tap', which addresses the user request, and not expose changing it to the users just yet. The tradeoff here is that increasing API surface means we ought to support it in the future (even if we change slider), so it becomes a maintenance burden. On the other hand, it might be useful enough to users to have a tap/snap/drag-snap behavior available. Are there usecases people actually see that do not want drag-tap behavior?

Thoughts? I know I've become very conservative about extending the core ipywidgets api because it aims to be very stable over a long term, so I'm wary of adding maintenance burdens.

@jasongrout
Copy link
Member

If exposing this behavior to the users is the way we want to go, then I think you've done a great job here (just reading the code, I haven't run it yet)

@ibdafna
Copy link
Member Author

ibdafna commented Mar 24, 2020

This PR adds functionality requested in #2683 .

#2832, right?

Indeed. Updated the PR note above with the correct one, thanks for spotting!

I was thinking it might be better to start out with just changing the default behavior to 'drag-tap', which addresses the user request, and not expose changing it to the users just yet. The tradeoff here is that increasing API surface means we ought to support it in the future (even if we change slider), so it becomes a maintenance burden. On the other hand, it might be useful enough to users to have a tap/snap/drag-snap behavior available. Are there usecases people actually see that do not want drag-tap behavior?

Potentially yes. Off the top of my head, let's say I'm using the range slider to control the spot range of a vanilla option payoff at expiry. I would probably not want to drag both handles of the slider at the same time given I am only looking to focus on the strike area. But then again, it might be convenient to drag the full range if I got the original parameters wrong and need to shift my window left or right.

Thoughts? I know I've become very conservative about extending the core ipywidgets api because it aims to be very stable over a long term, so I'm wary of adding maintenance burdens.

I'm definitely with you on robustness. I added the snap behaviour because I often expected the slider handles to behave that way and thought other users might also appreciate that option. If you think we are incurring potential technical debt down the line then I'm happy to remove the changes!

@vidartf
Copy link
Member

vidartf commented Mar 24, 2020

If you think we are incurring potential technical debt down the line then I'm happy to remove the changes!

We could also consider to "expose" it as an underscore param _behavior ("use at own risk"). 🤷‍♂

@ibdafna
Copy link
Member Author

ibdafna commented Mar 27, 2020

@jasongrout How should we proceed?

@jasongrout jasongrout added this to the 8.0 milestone Aug 10, 2021
@jtpio
Copy link
Member

jtpio commented Sep 14, 2021

@@ -159,6 +159,8 @@ class FloatSlider(_BoundedFloat):
default is 'horizontal', orientation of the slider
readout : {True, False}
default is True, display the current value of the slider next to it
behaviour : str
Copy link
Member

Choose a reason for hiding this comment

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

Commenting here as this came up during the weekly meeting today, with @ianhi spotting this attribute was not in American English:

Suggested change
behaviour : str
behavior : str

Copy link
Member

Choose a reason for hiding this comment

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

Ah that was probably because of noUISlider using behaviour in the options:

https://refreshless.com/nouislider/slider-options/#section-limit

image

@jtpio
Copy link
Member

jtpio commented Sep 17, 2021

FYI @ibdafna I rebased and added a commit to rename behaviour to behavior (American English)

@ibdafna
Copy link
Member Author

ibdafna commented Sep 17, 2021

@jtpio thank you 😄

@ibdafna ibdafna merged commit c7bd329 into jupyter-widgets:master Sep 17, 2021
@ibdafna ibdafna deleted the drag_behaviour branch September 17, 2021 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants