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

Create a RangeSlider widget #18829

Merged
merged 3 commits into from Dec 28, 2020
Merged

Create a RangeSlider widget #18829

merged 3 commits into from Dec 28, 2020

Conversation

ianhi
Copy link
Contributor

@ianhi ianhi commented Oct 28, 2020

PR Summary

Closes: #18563

Creates a SliderWidget that has a movable min and movable max. This enables doing things like this:
range_slider

The RangeSlider is as analogous to Slider with the major difference being that val is an array rather than a float. To accomplish this I made a new class _SliderBase that both the sliders inherit their callback and other shared functionality from.

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
    • code and tests
    • example - keeps complaining about the import for the references
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

examples/widgets/range_slider_demo.py Outdated Show resolved Hide resolved
lib/matplotlib/tests/test_widgets.py Outdated Show resolved Hide resolved
lib/matplotlib/widgets.py Outdated Show resolved Hide resolved
Comment on lines 722 to 725
s1 = super()._format(val[0])
s2 = super()._format(val[1])
# fmt.get_offset is actually the multiplicative factor, if any.
return (s1 + self._fmt.get_offset(), s2 + self._fmt.get_offset())
Copy link
Member

Choose a reason for hiding this comment

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

I think you may need to pass both values to format_ticks to ensure that the offset is consistent for both, though maybe this is okay due to self.slidermin and self.slidermax in the super class.

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 think it is saved by the slidermin and slidermax. It would be nice to pass both values simultaneously to format_ticks so I'll just make _format not part of the superclass. There has to be same branching on self.valfmt in RangeSlider anyway so theres not much loss in each slider implementing _format.

lib/matplotlib/widgets.py Outdated Show resolved Hide resolved
lib/matplotlib/widgets.py Outdated Show resolved Hide resolved
lib/matplotlib/widgets.py Outdated Show resolved Hide resolved
lib/matplotlib/widgets.py Outdated Show resolved Hide resolved
lib/matplotlib/widgets.py Outdated Show resolved Hide resolved
lib/matplotlib/widgets.py Outdated Show resolved Hide resolved
lib/matplotlib/widgets.py Outdated Show resolved Hide resolved
@ianhi
Copy link
Contributor Author

ianhi commented Dec 4, 2020

Rebased and added more explanatory text to the example + added a bit more functionality. The example now looks like this:

range-slider

I also tried out all the Slider examples and they still behave as expected.

Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

Just some docs are missing.

lib/matplotlib/widgets.py Outdated Show resolved Hide resolved
lib/matplotlib/widgets.py Outdated Show resolved Hide resolved
lib/matplotlib/widgets.py Outdated Show resolved Hide resolved
lib/matplotlib/widgets.py Outdated Show resolved Hide resolved
@ianhi
Copy link
Contributor Author

ianhi commented Dec 15, 2020

Is there any chance of this going into 3.4?

@QuLogic
Copy link
Member

QuLogic commented Dec 15, 2020

It needs a second reviewer.

@tacaswell tacaswell added this to the v3.4.0 milestone Dec 22, 2020
@QuLogic
Copy link
Member

QuLogic commented Dec 22, 2020

I just noticed that vertical is not tested; could you add tests that also check the other orientation? Depending on how things works, it may just be sufficient to parameterize the test for both.

@jklymak jklymak self-requested a review December 22, 2020 21:15
Also add a test of the polygon points to make sure that the slider is being rendered correctly.
@ianhi
Copy link
Contributor Author

ianhi commented Dec 23, 2020

I just noticed that vertical is not tested; could you add tests that also check the other orientation?

Good catch. I added a pytest parametrize to also test the vertical orientation, as well I added that the polygon's points are what we'd expect.

@timhoffm timhoffm merged commit 79ae33a into matplotlib:master Dec 28, 2020
@timhoffm
Copy link
Member

Thanks for the contribution! 👍

@ianhi ianhi deleted the range-slider branch March 25, 2021 23:33
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.

Create a RangeSlider widget
4 participants