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

new: repeat button for Play widget, can be hidden #1190

Merged
merged 5 commits into from Apr 26, 2017

Conversation

@maartenbreddels
Copy link
Member

commented Mar 7, 2017

As discussed in #1186 this adds a repeat button. I've chosed the fa-retweet button, since the fa-repeat does not look like a repeat button.
Futhermore I've included what I think is a fix for Play, it now includes max, since that is more consistent with the slider's max behavious.

import ipywidgets
p = ipywidgets.Play(min=0, max=3, interval=500, show_repeat=True)
s = ipywidgets.IntSlider(min=p.min, max=p.max, value=p.value)
ipywidgets.jslink((p, 'value'), (s, 'value'))
ipywidgets.HBox([p, s])

hide repeat button, and turn on repeat:

p.show_repeat = False
p._repeat = True
@SylvainCorlay

This comment has been minimized.

Copy link
Member

commented Mar 7, 2017

Awesome!

@maartenbreddels

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2017

Futhermore I've included what I think is a fix for Play, it now includes max, since that is more consistent with the slider's max behavious.

Is it an idea to take this in a seperate PR, since it will be nice to have this consistent for 7.0, and leave the extra button for 7.x or higher.
Since now Play goes from min to max-1, while a slider goes from min to max (inclusive), having this consistent makes sense.

@jasongrout

This comment has been minimized.

Copy link
Member

commented Apr 26, 2017

I think either way (fixing the consistency issue, or also including the new button) is fine for 7.0.

@jasongrout jasongrout added this to the 7.0 milestone Apr 26, 2017

@maartenbreddels

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2017

So, do you mind if I merge this then, I've tested it myself. Should the change in behaviour of Play go in some changelog?

@@ -660,12 +662,16 @@ class PlayModel extends BoundedIntModel {
loop() {
if (this.get('_playing')) {
var next_value = this.get('value') + this.get('step');
if (next_value < this.get('max')) {
if (next_value <= this.get('max')) {
this.set('value', next_value);
window.setTimeout(this.loop.bind(this), this.get('interval'));
} else {
this.set('value', this.get('min'));

This comment has been minimized.

Copy link
@jasongrout

jasongrout Apr 26, 2017

Member

Can we only reset it to the min if we are repeating? Reverting to the min after a straight play through has been frustrating to me, instead of just stopping at the end and freezing the state there.

This comment has been minimized.

Copy link
@maartenbreddels

maartenbreddels Apr 26, 2017

Author Member

I agree!

@jasongrout

This comment has been minimized.

Copy link
Member

commented Apr 26, 2017

@maartenbreddels

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2017

$#@% messed up rebasing, any idea what I did? :)

@maartenbreddels maartenbreddels force-pushed the maartenbreddels:repeat-button branch from 8eacee5 to 8e47708 Apr 26, 2017

@jasongrout

This comment has been minimized.

Copy link
Member

commented Apr 26, 2017

Looks like you figured it out.

I don't see that I can 'approve' sth

@maartenbreddels

This comment has been minimized.

Copy link
Member Author

commented Apr 26, 2017

Yes, I had to git push --force ...
Sorry I had to dismiss your review :), but I didn't see any other option.

@maartenbreddels maartenbreddels merged commit 760156a into jupyter-widgets:master Apr 26, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@maartenbreddels maartenbreddels referenced this pull request May 6, 2017
@jasongrout jasongrout referenced this pull request May 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.