undesirable '{0}' replacement

[pixelzoom]

Noted in https://github.com/phetsims/scenery-phet/issues/446.

NumberControl requires either '{{value}}' or '{0}' in the string pattern. The former is preferred, the latter is deprecated in new code.

These 2 strings are used with NumberControl:

  "gravityValue": {
    "value": "{{gravity}} m/s<sup>2</sup>"
  },

  "massValue": {
    "value": "{{mass}} g"
  },

If '{{value}}' had been used in those strings, these replacements would be unnecessary:

// GravityDampingControlNode
          valuePattern: StringUtils.fillIn( gravityValueString, {
            gravity: '{0}'
          } ),

// MassValueControlPanel
    var valuePattern = StringUtils.fillIn( massValueString, { mass: '{0}' }, {

Since these strings have been released and translated, it's probably too late (and too much work) to change them. But there's still room for improvement here.

Since NumberControl supports '{{value}}', replacement with '{0}' should be avoided. And proliferation of the string literals '{{value}}' and '{0}' should also be avoided. So recommended to replace '{0}' in the above with NumberDisplay.NAMED_PLACEHOLDER.

[Denz1994]

I made the refactor as suggested above and testing with several stringTest={TEST} query parameters. This looks good and should be cherry-picked into the RC branch of MASB. Labeling as such and referencing in RC Test issue.

[Denz1994]

@KatieWoe Reported that stringTest=long seems to be giving an issue on startup.

Exception: Error: Assertion failed: missing value placeholder in options.valuePattern: 12345678901234567890123456789012345678901234567890 at window.assertions.assertFunction (http://phettest.colorado.edu/assert/js/assert.js:22:13) at new NumberDisplay (http://phettest.colorado.edu/scenery-phet/js/NumberDisplay.js?bust=1551220939676:78:15) at new NumberControl (http://phettest.colorado.edu/scenery-phet/js/NumberControl.js?bust=1551220939676:179:25) at new GravityAndDampingControlNode (http://phettest.colorado.edu/masses-and-springs/js/common/view/GravityAndDampingControlNode.js?bust=1551220939676:79:32) at StretchScreenView.SpringScreenView [as constructor] (http://phettest.colorado.edu/masses-and-springs/js/common/view/SpringScreenView.js?bust=1551220939676:163:41) at new TwoSpringScreenView (http://phettest.colorado.edu/masses-and-springs/js/common/view/TwoSpringScreenView.js?bust=1551220939676:37:22) at new StretchScreenView (http://phettest.colorado.edu/masses-and-springs-basics/js/stretch/view/StretchScreenView.js?bust=1551220939676:33:7) at StretchScreen.model [as createView] (http://phettest.colorado.edu/masses-and-springs-basics/js/stretch/StretchScreen.js?bust=1551220939676:54:27) at StretchScreen.initializeView (http://phettest.colorado.edu/joist/js/Screen.js?bust=1551220939676:261:25) at Array. (http://phettest.colorado.edu/joist/js/Sim.js?bust=1551220939676:788:18) logMessage: "Assertion failed: missing value placeholder in options.valuePattern: 12345678901234567890123456789012345678901234567890" message: "missing value placeholder in options.valuePattern: 12345678901234567890123456789012345678901234567890" predicate: false this: undefined

[pixelzoom]

In https://github.com/phetsims/sun/issues/472#issuecomment-461165542, it was suggested that we add an assertion to NumberDisplay and NumberSpinner, to verify that the required {{value}} placeholder is in options.formatPattern. Unfortunately, stringTest is rather dumb when it comes to string replacement, and blows away the entire string. So that's why this assertion is failing only with stringTest.

I think the solution is to skip the assertion in NumberDisplay and NumberSpinner if phet.chipper.queryParameters.stringTest is defined. I'll handle this.

[pixelzoom]

Fixed in the above commits. Tested masses-and-springs with and without stringTest.

@Denz1994 please verify.

[Denz1994]

Thanks for looking into this. String test is looking good.

[Denz1994]

These changes have been applied to the masses-and-springs-basics-1.0 branch in the masses-and-springs repo. A branch inSun (`masses-and-springs-basics-1.0) was created to also handle these changes. MASB 1.0 sim dependencies have been updated.

Closing this issue