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

Use nested options for slider in NumberControl #451

Closed
zepumph opened this issue Dec 20, 2018 · 24 comments
Closed

Use nested options for slider in NumberControl #451

zepumph opened this issue Dec 20, 2018 · 24 comments

Comments

@zepumph
Copy link
Member

zepumph commented Dec 20, 2018

similar to #448, but for slider. This comment was discovered while investigating other work:

_.extend( _.omit( options, Node.prototype._mutatorKeys ), {

      // Use these more general callback options, because the same callbacks apply to the arrow buttons,
      // where it makes no sense to call them startDrag and endDrag.
      startDrag: options.sliderStartCallback || options.startCallback,
      endDrag: options.sliderEndCallback || options.endCallback,
      tandem: options.tandem.createTandem( 'slider' )
    } );

So all the options for NumberControl that aren't Node options are being propagated to the slider.

This has a few code smells.

  1. Why do all options need to go to the slider. What if I add a new option, it automatically gets passed to NumberControl.
  2. It assumes that the parent type is Node. If we changed this to instead extend Path, then Path options would still get passed to HSlider.
  3. This code was likely written back before we decided to adopt the nested options pattern. We should update to use that.
@zepumph
Copy link
Member Author

zepumph commented Dec 20, 2018

@mbarlow12 has been in the file recently, please let me know if you want my help!

@pixelzoom
Copy link
Contributor

pixelzoom commented Dec 23, 2018

If anyone needs an example of how to apply the "nested options" patterns, see FineCoarseSpinner or ComboDisplay.

Both demonstrate how to document nested options and set their default value. E.g. in FineCoarseSpinner:

numberDisplayOptions: null, // {*|null} options propagated to the NumberDisplay subcomponent

ComboDisplay demonstrates how to use an additional _.extends call to populate nested options with default values:

// defaults for NumberDisplay
options.numberDisplayOptions = _.extend( {
  ...
}, options.numberDisplayOptions );

FineCoarseSpinner demonstrates how to handle the case where the client is not allowed to set a nested option:

assert && assert( !options.arrowButtonOptions || options.arrowButtonOptions.numberOfArrows === undefined,
  'FineCoarseSpinner sets arrowButtonOptions.numberOfArrows' );

@pixelzoom
Copy link
Contributor

Since we nested options for ArrowButton in #448, and we're proposing to nest options for HSlider here... Why not also nest options for NumberDisplay? That would cover all 3 subcomponent types, and handle them uniformly.

options would contain these nested options:

// {*|null} options propagated to ArrowButton
arrowButtonOptions: null,

// {*|null} options propagated to HSlider
sliderOptions: null,

// {*|null} options propagated to NumberDisplay
numberDisplayOptions: null,

samreid added a commit to phetsims/circuit-construction-kit-common that referenced this issue Jan 29, 2019
mbarlow12 added a commit to phetsims/beers-law-lab that referenced this issue Feb 3, 2019
mbarlow12 added a commit to phetsims/circuit-construction-kit-common that referenced this issue Feb 3, 2019
mbarlow12 added a commit to phetsims/coulombs-law that referenced this issue Feb 3, 2019
mbarlow12 added a commit to phetsims/gravity-force-lab that referenced this issue Feb 3, 2019
mbarlow12 added a commit to phetsims/inverse-square-law-common that referenced this issue Feb 3, 2019
mbarlow12 added a commit to phetsims/hookes-law that referenced this issue Feb 3, 2019
mbarlow12 added a commit to phetsims/hookes-law that referenced this issue Feb 3, 2019
mbarlow12 added a commit to phetsims/hookes-law that referenced this issue Feb 3, 2019
mbarlow12 added a commit to phetsims/masses-and-springs that referenced this issue Feb 3, 2019
mbarlow12 added a commit to phetsims/pendulum-lab that referenced this issue Feb 3, 2019
mbarlow12 added a commit to phetsims/projectile-motion that referenced this issue Feb 3, 2019
mbarlow12 added a commit to phetsims/rutherford-scattering that referenced this issue Feb 3, 2019
mbarlow12 added a commit to phetsims/plinko-probability that referenced this issue Feb 3, 2019
@mbarlow12
Copy link
Contributor

@zepumph would you like to review? Whatever we decide on for phetsims/phet-info#91 will have an impact here.

@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 7, 2019

In NumberControl.js:

    // Defaults for NumberDisplay
    var numberDisplayOptions = _.extend( {
      // value
      font: new PhetFont( 12 ),
      align: 'right',
      valueMaxWidth: null, // {null|number} maxWidth to use for value display, to constrain width for i18n
      valueMinBackgroundWidth: 0, // {number} min width for the value display's background
      xMargin: 8,
      yMargin: 2,
      backgroundStroke: 'lightGray',
      backgroundLineWidth: 1,
      cornerRadius: 0,
      decimalPlaces: 0,
      useRichText: false,

      // {string} See NumberDisplay.valuePattern for additional requirements
      valuePattern: NumberDisplay.DEFAULT_VALUE_PATTERN,

      // phet-io
      tandem: options.tandem.createTandem( 'numberDisplay' )
    }, options.numberDisplayOptions );

Any of these default values that are the same as NumberDisplay's default values are redundant, and can be removed.

The redundant defaults are:

align: 'right',
xMargin: 8,
yMargin: 2,
backgroundStroke: 'lightGray',
cornerRadius: 0,
decimalPlaces: 0,
useRichText: false,
valuePattern: NumberDisplay.DEFAULT_VALUE_PATTERN,

Also, a potential problem... These options don't exist in NumberDisplay. Was this an existing problem, or a problem that was introduced?

valueMaxWidth: null, // {null|number} maxWidth to use for value display, to constrain width for i18n
valueMinBackgroundWidth: 0, // {number} min width for the value display's background

@pixelzoom
Copy link
Contributor

Btw... I looked at this because I'm doing related work in #446 and phetsims/sun#472. And getting rid of NumberDisplay.DEFAULT_VALUE_PATTERN is desirable in that work.

@zepumph
Copy link
Member Author

zepumph commented Jun 28, 2019

I saw usage of nested options, but not using the PHET_CORE/merge strategy.

I didn't know that was ready for production. I marked phetsims/phet-info#91 for developer meeting to understand that better. For now I'm going to try adding it in and just see how it goes. I also notice usages in wave-interference.

I saw checking for * Only general or specific callbacks are allowed, but not both. but it does not use the PHET_CORE/assertMutuallyExclusiveOptions.

I think that will be excellent. Thanks! Also see phetsims/phet-core#62

@zepumph
Copy link
Member Author

zepumph commented Jun 28, 2019

I saw checking for * Only general or specific callbacks are allowed, but not both. but it does not use the PHET_CORE/assertMutuallyExclusiveOptions.

I just realized this won't work, since one set of options is in options, and the other set of options is checked in options.arrowButtonOptions and options.sliderOptions. Still it is good to learn about that function, so thanks.

@zepumph
Copy link
Member Author

zepumph commented Jun 28, 2019

When it comes to merge. Many of the default nested options rely on options passed to the NumberControl. Here is a patch that extends some of the options, and then merges the rest of them. Perhaps we could make something like this work. like extend all options for NumberControl, and then do a single merge for all nested options (this patch doesn't quite get to that suggestion).

Index: js/NumberControl.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/NumberControl.js	(revision 453ea22f561260f39f4154b8f2b4ade6f84dd83d)
+++ js/NumberControl.js	(date 1561754989365)
@@ -19,6 +19,7 @@
   const HBox = require( 'SCENERY/nodes/HBox' );
   const HSlider = require( 'SUN/HSlider' );
   const inherit = require( 'PHET_CORE/inherit' );
+  const merge = require( 'PHET_CORE/merge' );
   const Node = require( 'SCENERY/nodes/Node' );
   const NumberControlIO = require( 'SCENERY_PHET/NumberControlIO' );
   const NumberDisplay = require( 'SCENERY_PHET/NumberDisplay' );
@@ -42,22 +43,36 @@
     'rightEnd'
   ];
   const POINTER_AREA_OPTION_NAMES = [ 'touchAreaXDilation', 'touchAreaYDilation', 'mouseAreaXDilation', 'mouseAreaYDilation' ];
-  const DEFAULT_CALLBACK = _.noop;
 
   /**
    * @param {string} title
    * @param {Property.<number>} numberProperty
    * @param {Range} numberRange
-   * @param {Object} [options] - subcomponent objects: sliderOptions, numberDisplayOptions, arrowButtonOptions, titleNodeOptions
+   * @param {Object} [options] - subcomponent options objects:
+   *                               sliderOptions,
+   *                               numberDisplayOptions,
+   *                               arrowButtonOptions,
+   *                               titleNodeOptions
    * @mixes AccessibleSlider
    * @constructor
    */
   function NumberControl( title, numberProperty, numberRange, options ) {
+
+    // Make sure that general callbacks (for all components) and specific callbacks (for a specific component) aren't
+    // used in tandem. This must be called before defaults are set.
+    validateCallbacks( options || {} );
+
     options = _.extend( {
-
       // General Callbacks
-      startCallback: DEFAULT_CALLBACK, // called when interaction begins, default value set in validateCallbacks()
-      endCallback: DEFAULT_CALLBACK, // called when interaction ends, default value set in validateCallbacks()
+      startCallback: _.noop, // called when interaction begins, default value set in validateCallbacks()
+      endCallback: _.noop, // called when interaction ends, default value set in validateCallbacks()
+
+      // phet-io
+      tandem: Tandem.required
+    } );
+
+    options = merge( {
+
       delta: 1,
 
       enabledProperty: new Property( true ), // {Property.<boolean>} is this control enabled?
@@ -70,117 +85,118 @@
       layoutFunction: NumberControl.createLayoutFunction1(),
 
       // {*|null} options propagated to ArrowButton
-      arrowButtonOptions: null,
-
-      // {*|null} options propagated to HSlider
-      sliderOptions: null,
-
-      // {*|null} options propagated to NumberDisplay
-      numberDisplayOptions: null,
-
-      // {*|null} options propagated to the title Text node
-      titleNodeOptions: null,
-
-      // phet-io
-      tandem: Tandem.required,
-      phetioType: NumberControlIO,
-
-      // a11y
-      groupFocusHighlight: true
-    }, options );
-
-    // validate options
-    assert && assert( !options.startDrag, 'use options.startCallback instead of options.startDrag' );
-    assert && assert( !options.endDrag, 'use options.endCallback instead of options.endDrag' );
-    assert && assert( options.disabledOpacity > 0 && options.disabledOpacity < 1,
-      `invalid disabledOpacity: ${options.disabledOpacity}` );
-    assert && assert( !options.shiftKeyboardStep,
-      'shift keyboard stop handled by arrow buttons, do not use with NumberControl' );
-    assert && assert( !options.shiftKeyboardStep,
-      'shift keyboard stop handled by arrow buttons, do not use with NumberControl' );
-    assert && options.sliderOptions && assert( options.isAccessible === undefined,
-      'NumberControl sets isAccessible for Slider' );
-
-    // Make sure that general callbacks (for all components) and specific callbacks (for a specific component) aren't
-    // used in tandem.
-    validateCallbacks( options );
-
-    // Defaults for ArrowButton
-    let arrowButtonOptions = _.extend( {
-      scale: 0.85,
+      arrowButtonOptions: {
+        scale: 0.85,
 
-      // Values chosen to match previous behavior, see https://github.com/phetsims/scenery-phet/issues/489.
-      // touchAreaXDilation is 1/2 of its original value because touchArea is shifted.
-      touchAreaXDilation: 3.5,
-      touchAreaYDilation: 7,
-      mouseAreaXDilation: 0,
-      mouseAreaYDilation: 0,
+        // Values chosen to match previous behavior, see https://github.com/phetsims/scenery-phet/issues/489.
+        // touchAreaXDilation is 1/2 of its original value because touchArea is shifted.
+        touchAreaXDilation: 3.5,
+        touchAreaYDilation: 7,
+        mouseAreaXDilation: 0,
+        mouseAreaYDilation: 0,
 
-      // callbacks
-      leftStart: options.startCallback, // called when left arrow is pressed
-      leftEnd: options.endCallback, // called when left arrow is released
-      rightStart: options.startCallback, // called when right arrow is pressed
-      rightEnd: options.endCallback // called when right arrow is released
-    }, options.arrowButtonOptions );
-
-    // Arrow button pointer areas need to be asymmetrical, see https://github.com/phetsims/scenery-phet/issues/489.
-    // Get the pointer area options related to ArrowButton so that we can handle pointer areas here.
-    // And do not propagate those options to ArrowButton instances.
-    const arrowButtonPointerAreaOptions = _.pick( arrowButtonOptions, POINTER_AREA_OPTION_NAMES );
-    arrowButtonOptions = _.omit( arrowButtonOptions, POINTER_AREA_OPTION_NAMES );
+        // callbacks
+        leftStart: options.startCallback, // called when left arrow is pressed
+        leftEnd: options.endCallback, // called when left arrow is released
+        rightStart: options.startCallback, // called when right arrow is pressed
+        rightEnd: options.endCallback // called when right arrow is released
+      },
 
-    // a11y - for alternative input, the number control is accessed entirely through slider interaction and these
-    // arrow buttons are not tab navigable
-    assert && assert( arrowButtonOptions.tagName === undefined,
-      'NumberControl handles alternative input for arrow buttons' );
-    arrowButtonOptions.tagName = null;
-
-    // Defaults for HSlider
-    let sliderOptions = _.extend( {
+      // {*|null} options propagated to HSlider
+      sliderOptions: {
 
-      startDrag: options.startCallback, // called when dragging starts on the slider
-      endDrag: options.endCallback, // called when dragging ends on the slider
+        startDrag: options.startCallback, // called when dragging starts on the slider
+        endDrag: options.endCallback, // called when dragging ends on the slider
 
-      // With the exception of startDrag and endDrag (use startCallback and endCallback respectively),
-      // all HSlider options may be used. These are the ones that NumberControl overrides:
-      majorTickLength: 20,
-      minorTickStroke: 'rgba( 0, 0, 0, 0.3 )',
+        // With the exception of startDrag and endDrag (use startCallback and endCallback respectively),
+        // all HSlider options may be used. These are the ones that NumberControl overrides:
+        majorTickLength: 20,
+        minorTickStroke: 'rgba( 0, 0, 0, 0.3 )',
 
-      // other slider options that are specific to NumberControl
-      majorTicks: [], // array of objects with these fields: { value: {number}, label: {Node} }
-      minorTickSpacing: 0, // zero indicates no minor ticks
+        // other slider options that are specific to NumberControl
+        majorTicks: [], // array of objects with these fields: { value: {number}, label: {Node} }
+        minorTickSpacing: 0, // zero indicates no minor ticks
 
-      // constrain the slider value to the provided range and the same delta as the arrow buttons
-      constrainValue: value => {
-        const newValue = Util.roundToInterval( value, options.delta ); // constrain to multiples of delta, see #384
-        return numberRange.constrainValue( newValue );
-      },
+        // constrain the slider value to the provided range and the same delta as the arrow buttons
+        constrainValue: value => {
+          const newValue = Util.roundToInterval( value, options.delta ); // constrain to multiples of delta, see #384
+          return numberRange.constrainValue( newValue );
+        },
 
-      // phet-io
-      tandem: options.tandem.createTandem( 'slider' )
-    }, options.sliderOptions );
+        // phet-io
+        tandem: options.tandem.createTandem( 'slider' )
+      },
+
+      // {*|null} options propagated to NumberDisplay
+      numberDisplayOptions: {
+        // value
+        font: new PhetFont( 12 ),
+        maxWidth: null, // {null|number} maxWidth to use for value display, to constrain width for i18n
+
+        // phet-io
+        tandem: options.tandem.createTandem( 'numberDisplay' )
+      },
+
+      // {*|null} options propagated to the title Text node
+      titleNodeOptions: {
+        font: new PhetFont( 12 ),
+        maxWidth: null, // {null|string} maxWidth to use for title, to constrain width for i18n
+        fill: 'black',
+        tandem: options.tandem.createTandem( 'titleNode' )
+      },
+
+      // phet-io
+      phetioType: NumberControlIO,
+
+      // a11y
+      groupFocusHighlight: true
+    }, options );
+
+    // validate options
+    assert && assert( !options.startDrag, 'use options.startCallback instead of options.startDrag' );
+    assert && assert( !options.endDrag, 'use options.endCallback instead of options.endDrag' );
+    assert && assert( options.disabledOpacity > 0 && options.disabledOpacity < 1,
+      `invalid disabledOpacity: ${options.disabledOpacity}` );
+    assert && assert( !options.shiftKeyboardStep,
+      'shift keyboard stop handled by arrow buttons, do not use with NumberControl' );
+    assert && assert( !options.shiftKeyboardStep,
+      'shift keyboard stop handled by arrow buttons, do not use with NumberControl' );
+    assert && options.sliderOptions && assert( options.isAccessible === undefined,
+      'NumberControl sets isAccessible for Slider' );
+
+    // Arrow button pointer areas need to be asymmetrical, see https://github.com/phetsims/scenery-phet/issues/489.
+    // Get the pointer area options related to ArrowButton so that we can handle pointer areas here.
+    // And do not propagate those options to ArrowButton instances.
+    const arrowButtonPointerAreaOptions = _.pick( options.arrowButtonOptions, POINTER_AREA_OPTION_NAMES );
+    options.arrowButtonOptions = _.omit( options.arrowButtonOptions, POINTER_AREA_OPTION_NAMES );
+
+    // a11y - for alternative input, the number control is accessed entirely through slider interaction and these
+    // arrow buttons are not tab navigable
+    assert && assert( options.arrowButtonOptions.tagName === undefined,
+      'NumberControl handles alternative input for arrow buttons' );
+    options.arrowButtonOptions.tagName = null;
 
     // Slider options for track (if not specified as trackNode)
     if ( !options.sliderOptions.trackNode ) {
-      sliderOptions = _.extend( {
+      options.sliderOptions = _.extend( {
         trackSize: new Dimension2( 180, 3 )
-      }, sliderOptions );
+      }, options.sliderOptions );
     }
 
-    // Slider options for thumb (if not specified as thumbNode)
+    // Slider options for thumb (if n ot specified as thumbNode)
     if ( !options.sliderOptions.thumbNode ) {
-      sliderOptions = _.extend( {
+      options.sliderOptions = _.extend( {
         thumbSize: new Dimension2( 17, 34 ),
         thumbTouchAreaXDilation: 6
-      }, sliderOptions );
+      }, options.sliderOptions );
     }
 
-    assert && assert( !sliderOptions.hasOwnProperty( 'isAccessible' ), 'NumberControl sets isAccessible' );
-    assert && assert( !sliderOptions.hasOwnProperty( 'shiftKeyboardStep' ), 'NumberControl sets shiftKeyboardStep' );
-    assert && assert( !sliderOptions.hasOwnProperty( 'phetioType' ), 'NumberControl sets phetioType' );
+    assert && assert( !options.sliderOptions.hasOwnProperty( 'isAccessible' ), 'NumberControl sets isAccessible' );
+    assert && assert( !options.sliderOptions.hasOwnProperty( 'shiftKeyboardStep' ), 'NumberControl sets shiftKeyboardStep' );
+    assert && assert( !options.sliderOptions.hasOwnProperty( 'phetioType' ), 'NumberControl sets phetioType' );
 
     // slider options set by NumberControl, note this may not be the long term pattern, see https://github.com/phetsims/phet-info/issues/96
-    sliderOptions = _.extend( {
+    options.sliderOptions = _.extend( {
 
       // NumberControl uses the AccessibleSlider trait, so don't include any accessibility on the slider
       isAccessible: false,
@@ -190,38 +206,21 @@
 
       // Make sure Slider gets created with the right IO Type
       phetioType: SliderIO
-    }, sliderOptions );
+    }, options.sliderOptions );
 
     // highlight color for thumb defaults to a brighter version of the thumb color
-    if ( sliderOptions.thumbFill && !sliderOptions.thumbFillHighlighted ) {
+    if ( options.sliderOptions.thumbFill && !options.sliderOptions.thumbFillHighlighted ) {
 
       // @private {Property.<Color>}
-      this.thumbFillProperty = new PaintColorProperty( sliderOptions.thumbFill );
+      this.thumbFillProperty = new PaintColorProperty( options.sliderOptions.thumbFill );
 
       // Reference to the DerivedProperty not needed, since we dispose what it listens to above.
-      sliderOptions.thumbFillHighlighted = new DerivedProperty( [ this.thumbFillProperty ], color => color.brighterColor() );
+      options.sliderOptions.thumbFillHighlighted = new DerivedProperty( [ this.thumbFillProperty ], color => color.brighterColor() );
     }
 
-    // Defaults for NumberDisplay
-    const numberDisplayOptions = _.extend( {
-      // value
-      font: new PhetFont( 12 ),
-      maxWidth: null, // {null|number} maxWidth to use for value display, to constrain width for i18n
-
-      // phet-io
-      tandem: options.tandem.createTandem( 'numberDisplay' )
-    }, options.numberDisplayOptions );
+    const titleNode = new Text( title, options.titleNodeOptions );
 
-    const titleNodeOptions = _.extend( {
-      font: new PhetFont( 12 ),
-      maxWidth: null, // {null|string} maxWidth to use for title, to constrain width for i18n
-      fill: 'black',
-      tandem: options.tandem.createTandem( 'titleNode' )
-    }, options.titleNodeOptions );
-
-    const titleNode = new Text( title, titleNodeOptions );
-
-    const numberDisplay = new NumberDisplay( numberProperty, numberRange, numberDisplayOptions );
+    const numberDisplay = new NumberDisplay( numberProperty, numberRange, options.numberDisplayOptions );
 
     const leftArrowButton = new ArrowButton( 'left', () => {
       let value = numberProperty.get() - options.delta;
@@ -229,10 +228,10 @@
       value = Math.max( value, numberRange.min ); // constrain to range
       numberProperty.set( value );
     }, _.extend( {
-      startCallback: arrowButtonOptions.leftStart,
-      endCallback: arrowButtonOptions.leftEnd,
+      startCallback: options.arrowButtonOptions.leftStart,
+      endCallback: options.arrowButtonOptions.leftEnd,
       tandem: options.tandem.createTandem( 'leftArrowButton' )
-    }, arrowButtonOptions ) );
+    }, options.arrowButtonOptions ) );
 
     const rightArrowButton = new ArrowButton( 'right', () => {
       let value = numberProperty.get() + options.delta;
@@ -240,10 +239,10 @@
       value = Math.min( value, numberRange.max ); // constrain to range
       numberProperty.set( value );
     }, _.extend( {
-      startCallback: arrowButtonOptions.rightStart,
-      endCallback: arrowButtonOptions.rightEnd,
+      startCallback: options.arrowButtonOptions.rightStart,
+      endCallback: options.arrowButtonOptions.rightEnd,
       tandem: options.tandem.createTandem( 'rightArrowButton' )
-    }, arrowButtonOptions ) );
+    }, options.arrowButtonOptions ) );
 
     // arrow button touchAreas, asymmetrical, see https://github.com/phetsims/scenery-phet/issues/489
     leftArrowButton.touchArea = leftArrowButton.localBounds
@@ -267,21 +266,21 @@
     };
     numberProperty.link( arrowEnabledListener );
 
-    const slider = new HSlider( numberProperty, numberRange, sliderOptions );
+    const slider = new HSlider( numberProperty, numberRange, options.sliderOptions );
 
     // major ticks
-    const majorTicks = sliderOptions.majorTicks;
+    const majorTicks = options.sliderOptions.majorTicks;
     for ( let i = 0; i < majorTicks.length; i++ ) {
       slider.addMajorTick( majorTicks[ i ].value, majorTicks[ i ].label );
     }
 
     // minor ticks, exclude values where we already have major ticks
-    if ( sliderOptions.minorTickSpacing > 0 ) {
+    if ( options.sliderOptions.minorTickSpacing > 0 ) {
       for ( let minorTickValue = numberRange.min; minorTickValue <= numberRange.max; ) {
         if ( !_.find( majorTicks, majorTick => majorTick.value === minorTickValue ) ) {
           slider.addMinorTick( minorTickValue );
         }
-        minorTickValue += sliderOptions.minorTickSpacing;
+        minorTickValue += options.sliderOptions.minorTickSpacing;
       }
     }
 
@@ -294,7 +293,7 @@
     Node.call( this, options );
 
     // a11y - the number control acts like a range input for a11y, pass slider options without tandem
-    const accessibleSliderOptions = _.omit( sliderOptions, [ 'tandem' ] );
+    const accessibleSliderOptions = _.omit( options.sliderOptions, [ 'tandem' ] );
     this.initializeAccessibleSlider( numberProperty, slider.enabledRangeProperty, slider.enabledProperty, accessibleSliderOptions );
 
     // a11y - the focus highlight for NumberControl should surround the Slider's thumb
@@ -350,8 +349,8 @@
    * @param {Object} options
    */
   function validateCallbacks( options ) {
-    const normalCallbacksPresent = !!( options.startCallback !== DEFAULT_CALLBACK ||
-                                       options.endCallback !== DEFAULT_CALLBACK );
+    const normalCallbacksPresent = !!( options.startCallback ||
+                                       options.endCallback );
     let arrowCallbacksPresent = false;
     let sliderCallbacksPresent = false;
 

@zepumph
Copy link
Member Author

zepumph commented Jun 28, 2019

@samreid please review the above comments. After the two investigations above. I would be fine either keeping things the way they are, or investigating merging all nested options in a single call.

@zepumph
Copy link
Member Author

zepumph commented Jun 28, 2019

Hey sorry for the stream of consciousness above. I actually ended up implementing the proposal in #451 (comment).

It worked well to have an extend call for all options for NumberControl, and then a second merge for all nested options at once.

Though this doesn't simplify things into a single call, it get's closer than X extend calls, one for each nested object.

@samreid please review.

@zepumph zepumph assigned samreid and unassigned zepumph Jun 28, 2019
@samreid
Copy link
Member

samreid commented Jun 28, 2019

@zepumph and @chrisklus reviewed the changes and the preceding comments today, and all seems well. Closing.

@samreid samreid closed this as completed Jun 28, 2019
@samreid samreid reopened this Jun 29, 2019
@samreid
Copy link
Member

samreid commented Jun 29, 2019

CT is showing some issues likely related to these changes.

beers-law-lab : phet-io-fuzz : require.js : load
Query: brand=phet-io&phetioStandalone&ea&phetioValidateTandems&fuzz&memoryLimit=1000
Uncaught Error: Assertion failed: Tandem was required but not supplied
Error: Assertion failed: Tandem was required but not supplied
    at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/snapshot-1561781437006/assert/js/assert.js:22:13)
    at Tandem.addPhetioObject (https://bayes.colorado.edu/continuous-testing/snapshot-1561781437006/tandem/js/Tandem.js?bust=1561792925816:115:21)
    at Text.initializePhetioObject (https://bayes.colorado.edu/continuous-testing/snapshot-1561781437006/tandem/js/PhetioObject.js?bust=1561792925816:306:19)
    at Text.mutate (https://bayes.colorado.edu/continuous-testing/snapshot-1561781437006/scenery/js/nodes/Node.js?bust=1561792925816:4990:12)
    at Text.Node [as constructor] (https://bayes.colorado.edu/continuous-testing/snapshot-1561781437006/scenery/js/nodes/Node.js?bust=1561792925816:526:12)
    at new Text (https://bayes.colorado.edu/continuous-testing/snapshot-1561781437006/scenery/js/nodes/Text.js?bust=1561792925816:90:10)
    at ConcentrationControl.NumberControl [as constructor] (https://bayes.colorado.edu/continuous-testing/snapshot-1561781437006/scenery-phet/js/NumberControl.js?bust=1561792925816:220:23)
    at new ConcentrationControl (https://bayes.colorado.edu/continuous-testing/snapshot-1561781437006/beers-law-lab/js/beerslaw/view/ConcentrationControl.js?bust=1561792925816:135:19)
    at https://bayes.colorado.edu/continuous-testing/snapshot-1561781437006/beers-law-lab/js/beerslaw/view/SolutionControls.js?bust=1561792925816:46:15
    at Array.map (<anonymous>)
id: Bayes Chrome
Approximately 6/28/2019, 10:10:37 PM

samreid added a commit that referenced this issue Jun 29, 2019
samreid added a commit to phetsims/energy-skate-park that referenced this issue Jun 29, 2019
@samreid
Copy link
Member

samreid commented Jun 29, 2019

In the preceding 2 commits, I fixed the NumberControl options extend and fixed a typo in energy-skate-park. The following commits will be the results of grunt update.

samreid added a commit to phetsims/gravity-force-lab that referenced this issue Jun 29, 2019
samreid added a commit to phetsims/energy-skate-park that referenced this issue Jun 29, 2019
samreid added a commit to phetsims/beers-law-lab that referenced this issue Jun 29, 2019
samreid added a commit to phetsims/coulombs-law that referenced this issue Jun 29, 2019
samreid added a commit to phetsims/hookes-law that referenced this issue Jun 29, 2019
@samreid
Copy link
Member

samreid commented Jun 29, 2019

@zepumph can you please review the changes?

@samreid samreid assigned zepumph and unassigned samreid Jun 29, 2019
@zepumph
Copy link
Member Author

zepumph commented Jul 2, 2019

This looks great thank you. CT has been clear for a few days now. I appreciate the cleanup!

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

4 participants