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

fix: support ipywidget sliders in save_animation #182

Merged
merged 6 commits into from
Apr 27, 2021
Merged

fix: support ipywidget sliders in save_animation #182

merged 6 commits into from
Apr 27, 2021

Conversation

redeboer
Copy link
Contributor

Closes #181

These are the changes required to get the snippet from #181 to work. Feel free to close this PR if it has any undesired consequences or does not comply with the design of the package.

Copy link
Collaborator

@ianhi ianhi left a comment

Choose a reason for hiding this comment

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

Ok I've thought about this a little bit more. I think the underlying issue is that I lumped the test for being ipywidgets together with the test for if it was automatically created. This is leading to you duplicating the N,min_... code. I think a more reasonable flow would be:

# test if this is an autogenerated slider
if "Box" in str(slider...)
    # loop over children of the box
          slider =  ....

if not _not_ipython and isinstance(slider, [FloatSlider, IntSlider):
   ...
elif isinstance(slider, mSlider):
   ...

Would you mind changing the implementation to be like that? (Or convince me that something else is better!)

Ideally none of these should throw an error
@ianhi
Copy link
Collaborator

ianhi commented Apr 27, 2021

I took the liberty of adding a test with mixed slider types to make sure that they all work.

@redeboer
Copy link
Contributor Author

#182 (comment)
Ah perfect, thanks!

#182 (review)
I made this of it now bf85263, but it's not perfect. Could be this is related to the abstract slider idea described in #183. If slider here
https://github.com/ianhi/mpl-interactions/blob/ef9b0a9c83983f94ead50aa3e36a54314d76e63b/mpl_interactions/controller.py#L276
had some common interface with a min, max, and step, that could simplify the code here. But for now, hope this suffices.

@ianhi
Copy link
Collaborator

ianhi commented Apr 27, 2021

oooh hooray the tests caught that! they are good for something 🎉

@ianhi ianhi merged commit 3708dda into mpl-extensions:master Apr 27, 2021
@ianhi
Copy link
Collaborator

ianhi commented Apr 27, 2021

Thanks @redeboer ! I'll make a new release shortly.

@all-contributors please add @redeboer for code and userTesting

@allcontributors
Copy link
Contributor

@ianhi

I've put up a pull request to add @redeboer! 🎉

@redeboer redeboer deleted the ipywidgets-sliders branch April 27, 2021 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle ipywidget sliders in Controls.save_animation
2 participants