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 a way to listen for Slider drag events #561

Closed
samreid opened this issue Feb 3, 2020 · 12 comments
Closed

Add a way to listen for Slider drag events #561

samreid opened this issue Feb 3, 2020 · 12 comments

Comments

@samreid
Copy link
Member

samreid commented Feb 3, 2020

As part of phetsims/wave-interference#468 we are adding discrete slider clicking sounds to Wave Interference. The demonstration for this in tambo wires up listeners to the Properties directly,

      const sliderDecreaseClickSoundClip = new SoundClip( sliderDecreaseClickSound, {
        initiateWhenDisabled: false,
        enableControlProperties: [ resetNotInProgressProperty ]
      } );
      soundManager.addSoundGenerator( sliderDecreaseClickSoundClip );
      model.discreteValueProperty.lazyLink( ( newValue, oldValue ) => {
        if ( newValue > oldValue ) {
          sliderIncreaseClickSoundClip.play();
        }
        else {
          sliderDecreaseClickSoundClip.play();
        }
      } );

However, I recall a conversation where we agreed that we would couple the sounds to the user interface events directly, rather than going through the controlled Property instances. It looks like Slider has options for startDrag and endDrag but not for drag. In order to power the sound events by user interface events, we will need a way to listen to drag.

@pixelzoom does this sound appropriate to you? Should we add options.drag to Slider?

@pixelzoom
Copy link
Contributor

Yep, sounds appropriate to me (no pun intended :)

@samreid
Copy link
Member Author

samreid commented Feb 3, 2020

Added in the commit. @pixelzoom would you like to review?

@pixelzoom
Copy link
Contributor

The doc for option drag is:

drag: _.noop, // called on drag

Shouldn't it say something about when it's called on drag? That would be expected for "hook" documentation. Looks like it's called first, before any other drag work happens.

@samreid
Copy link
Member Author

samreid commented Feb 3, 2020

I added documentation, ready for review.

@samreid samreid assigned pixelzoom and unassigned samreid Feb 3, 2020
@pixelzoom
Copy link
Contributor

👍 closing.

@zepumph zepumph reopened this Apr 16, 2020
@zepumph
Copy link
Member

zepumph commented Apr 16, 2020

While looking at phetsims/wave-interference#484 I was curious as to why we decided to call drag first. Can you clarify? Looking around at other usages of "callbacks" on the project make me feel like it makes more sense to call it at the very end, after the Property has been updated. I see callbacks called last in PressListener, and SimpleDragListener, and KeyboardDragListener.

const mySlider = new Slider( myProperty, {
  drag: ()=>{ console.log( myProperty.value, 'value for this drag event' ); }
} );

This code is not logging the right value, currently.

Note, I was surprised to see that AccessibleValueHandler is also calling callbacks earlier rather than later. Do we want a new issue for that?

@samreid
Copy link
Member Author

samreid commented Apr 17, 2020

I marked this as blocking for Waves Intro

@jessegreenberg
Copy link
Contributor

#561 (comment) Makes sense to me.

Note, I was surprised to see that AccessibleValueHandler is also calling callbacks earlier rather than later. Do we want a new issue for that

I was trying to match the behavior of Slider in AccessibleValueHandler, if the callback moves in Slider.js it should move in the AccessibleValueHandler trait as well.

@jessegreenberg
Copy link
Contributor

We discussed this yesterday during developer meeting and agreed that the optional drag callback should be moved to after the Property value is set.

@jessegreenberg
Copy link
Contributor

The optional callback was moved, @samreid can you please review?

@jessegreenberg
Copy link
Contributor

Oh and I should mention that WaveInterferenceSlider was the only place I could find that used the drag option which made this change feel pretty safe.

@samreid
Copy link
Member Author

samreid commented May 12, 2020

I reviewed the change and tested in Wave Interference and it is working nicely, thanks! Closing.

@samreid samreid closed this as completed May 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants