use nested options for CoefficientSliderNode

[pixelzoom]

Related to code review #143.

CoefficientSliderNode is a good opportunity to use nested options. The first part of its constructor looks like this:

    /**
     * @param {Property.<number>} property parameter to track.
     * @param {Range} range - Possible range for property.
     * @param {string} label - Label for slider.
     * @param {Object} [options] for slider node.
     */
    constructor( property, range, label, options ) {
      options = _.extend( {
        trackFill: 'black',
        trackSize: new Dimension2( 120, 1 ),
        thumbSize: new Dimension2( 10, 20 ),
        minorTickLineWidth: 2,
        minorTickLength: 12,
        thumbTouchAreaYDilation: 4, // supposed to make touch horizontal areas flush; is YDilation since we rotate by 90; see #72
        thumbMouseAreaYDilation: 4,
        thumbMouseAreaXDilation: 10,
        labelOptions: {
          font: new PhetFont( {
            weight: 'bold',
            size: 13
          } ),
          fill: CurveFittingConstants.BLUE_COLOR,
          maxWidth: 20
        }
      }, options );

The doc "@param {Object} [options] for slider node" is incorrect, because labelOptions is not for Slider.

Something like this would be better. Note the use of merge and sliderOptions.

    /**
     * @param {Property.<number>} property parameter to track.
     * @param {Range} range - Possible range for property.
     * @param {string} label - Label for slider.
     * @param {Object} [options] for slider node.
     */
    constructor( property, range, label, options ) {
      options = merge( {
        sliderOptions: {
          trackFill: 'black',
          trackSize: new Dimension2( 120, 1 ),
          thumbSize: new Dimension2( 10, 20 ),
          minorTickLineWidth: 2,
          minorTickLength: 12,
          thumbTouchAreaYDilation: 4, // supposed to make touch horizontal areas flush; is YDilation since we rotate by 90; see #72
          thumbMouseAreaYDilation: 4,
          thumbMouseAreaXDilation: 10
        },
        labelOptions: {
          font: new PhetFont( {
            weight: 'bold',
            size: 13
          } ),
          fill: CurveFittingConstants.BLUE_COLOR,
          maxWidth: 20
        }
      }, options );

[SaurabhTotey]

I switched to the more consistent nesting and use of merge. Assigning to @pixelzoom to review.

[pixelzoom]

The label font got reverted in https://github.com/phetsims/curve-fitting/commit/dee6c345a2a35ba6eb785a529ad698acc19aa50b. I had changed it to CurveFittingConstants.COEFFICIENT_FONT as part of #157, and you set it back to font: new PhetFont( { weight: 'bold', size: 13 } ). So I've fixed that in the above commit.

[pixelzoom]

Sorry that I didn't notice before, but the options arg to CoefficientSliderNode is never used. The single use of CoefficientSliderNode is in FitPanel:

      const ascendingSliders = slidersAttributes.map(
        ( sliderAttribute, index ) => new CoefficientSliderNode(
          sliderPropertyArray[ index ],
          sliderAttribute.range,
          sliderAttribute.string,
          { enabledProperty: sliderAttribute.enabledProperty }
        )
      );

So in this case, there's no need to have any options related to the Slider or Text label. Simply pass the desired options to the Slider and the Text. I've made that change in the above commit. This also saves a tiny bit of memory, since the options literals are not created once, instead of for each instance of CoefficientSliderNode.

Back to @SaurabhTotey for review. Feel free to close if all looks well.

[pixelzoom]

Hmm... There's something else that's strange here, so I've restored sliderOptions and labelOptions in the above commits.

In FitPanel:

      // attributes for four sliders in ASCENDING order of polynomial
      const slidersAttributes = [
        {
          string: dSymbolString,
          range: CurveFittingConstants.CONSTANT_RANGE,
          enabledProperty: new BooleanProperty( true )
        },
        {
          string: cSymbolString,
          range: CurveFittingConstants.LINEAR_RANGE,
          enabledProperty: new BooleanProperty( true )
        },
        {
          string: bSymbolString,
          range: CurveFittingConstants.QUADRATIC_RANGE,
          enabledProperty: new BooleanProperty( true )
        },
        {
          string: aSymbolString,
          range: CurveFittingConstants.CUBIC_RANGE,
          enabledProperty: new BooleanProperty( true )
        }
      ];

      // create array in ASCENDING order of polynomial
      const ascendingSliders = slidersAttributes.map(
        ( sliderAttribute, index ) => new CoefficientSliderNode(
          sliderPropertyArray[ index ],
          sliderAttribute.range,
          sliderAttribute.string,
          { enabledProperty: sliderAttribute.enabledProperty }
        )
      );

Where is the enabledProperty option value being used? CoefficientSliderNode and its subclass VBox have no such option. And while using this sim, I haven't run into any situation where a slider is disabled.

[pixelzoom]

It looks to me like enabledProperty is unused and can be delete from slidersAttributes and the options to new CoefficientSliderNode.

[SaurabhTotey]

Looking in FitPanel on lines 138 and 139, I see this comment

// if the sliders are not disabled they will be able to change
// and behave as described in #15 and #37

I don't know if this still holds true, so I will need to test on a multitouch device before deleting enabledProperty.

[SaurabhTotey]

After doing some testing, it looks like the enabledProperty didn't do anything, so I removed it. Closing this issue now.