phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

Name and docs for the onChange option in AccessibleValueHandler don't match behavior #760

Closed jbphet closed 2 years ago

jbphet commented 2 years ago

While working on https://github.com/phetsims/sun/issues/759, I noticed that the onChange listener in AccessibleValueHandler handler is sometimes called when no change to the value occurs, such as when the value is clamped at a boundary. The documentation for this option currently looks like this:

  // Called after any change to valueProperty. Useful for input devices that support "press and hold" input.
  // However, beware that some input devices, such as a switch, have no concept of "press and hold" and will
  // trigger once per input. In those cases, this function will be called once per input.
  onChange?: SceneryListenerFunction;

At first I thought this was a bug and proposed modifying the behavior to only invoke the function on an actual change of value, but when discussing it with @zepumph and @jessegreenberg, @zepumph said that he definitely has code that would break if we were to make this modification.

IMO, we should change this to something like onInput and modify the docs to be correct in order to make the API easier to use and to avoid problems in the future.

zepumph commented 2 years ago

@jbphet, I renamed things above. I didn't want to have the API be startChange and onInput, so it is startInput, onInput, and endInput now. I tried to fix doc whenever I found it too. Please review and feel free to change doc to what makes sense to you.

jbphet commented 2 years ago

Looks good, thanks for doing this. I also updated some of the documentation in NumberPicker to be more consistent with this new set of names. Closing.