Adressing Review comments, see #40

phetsims · Aug 14, 2023 · 48212d7 · 48212d7
1 parent 0c8b987
commit 48212d7
Show file tree

Hide file tree

Showing 4 changed files with 19 additions and 37 deletions.
diff --git a/js/KeplersLawsStrings.ts b/js/KeplersLawsStrings.ts
@@ -69,7 +69,7 @@ type StringsType = {
     'valueUnitsStringProperty': LocalizedStringProperty;
   };
   'warning': {
-    'warningStringProperty': LocalizedStringProperty;
+    'warningPatternStringProperty': LocalizedStringProperty;
     'crashOrbitStringProperty': LocalizedStringProperty;
     'escapeOrbitStringProperty': LocalizedStringProperty;
   };

diff --git a/js/common/view/KeplersLawsOrbitalInformationBox.ts b/js/common/view/KeplersLawsOrbitalInformationBox.ts
@@ -113,9 +113,7 @@ class KeplersLawsOrbitalInformationBox extends VBox {
       createCheckbox(
         model.periapsisVisibleProperty,
         KeplersLawsStrings.periapsisStringProperty,
-        //REVIEW: visibFle looks like a typo introduced in https://github.com/phetsims/my-solar-system/commit/8ed18445210b1f2fbfdc88759cdb0894b8a3004a
-        //REVIEW: it should be fixed
-        'periapsisVisibFleCheckbox',
+        'periapsisVisibleCheckbox',
         new XNode( {
           fill: 'gold',
           stroke: SolarSystemCommonColors.foregroundProperty,

diff --git a/js/common/view/OrbitalWarningMessage.ts b/js/common/view/OrbitalWarningMessage.ts
@@ -13,10 +13,10 @@ import ModelViewTransform2 from '../../../../phetcommon/js/view/ModelViewTransfo
 import Vector2 from '../../../../dot/js/Vector2.js';
 import OrbitTypes from '../model/OrbitTypes.js';
 import KeplersLawsStrings from '../../KeplersLawsStrings.js';
-import Multilink from '../../../../axon/js/Multilink.js';
 import keplersLaws from '../../keplersLaws.js';
 import SolarSystemCommonConstants from '../../../../solar-system-common/js/SolarSystemCommonConstants.js';
 import { combineOptions } from '../../../../phet-core/js/optionize.js';
+import StringUtils from '../../../../phetcommon/js/util/StringUtils.js';
 
 export default class OrbitalWarningMessage extends Node {
 
@@ -26,45 +26,29 @@ export default class OrbitalWarningMessage extends Node {
 
     const center = modelViewTransformProperty.value.modelToViewPosition( new Vector2( 0, -50 ) );
 
-    //REVIEW: Why a local variable for this?
-    //REVIEW: Please create a DerivedProperty<string> for this, and then pass that to the RichText constructor 1st param
-    //REVIEW: instead of the multilink that is used.
-    //REVIEW: no need for `message` variable
-    let message = '';
-    const warningText = new RichText( message, combineOptions<TextOptions>( {
-      maxWidth: 500
-    }, SolarSystemCommonConstants.TITLE_OPTIONS ) );
-
-
-    Multilink.multilink(
+    const messageProperty = new DerivedProperty(
       [
         orbitTypeProperty,
-        KeplersLawsStrings.warning.warningStringProperty,
+        KeplersLawsStrings.warning.warningPatternStringProperty,
         KeplersLawsStrings.warning.crashOrbitStringProperty,
         KeplersLawsStrings.warning.escapeOrbitStringProperty
       ],
       ( orbitType, warningString, crashOrbitString, escapeOrbitString ) => {
-        //REVIEW: I believe i18n patterns should be used for this, instead of string concatenation with a colon
-        //REVIEW: why is the colon, space, and placeholder not available in the warning.warning string?
-        message = warningString + ': ';
-        switch( orbitType ) {
-          case OrbitTypes.CRASH_ORBIT:
-            message += crashOrbitString;
-            break;
-          case OrbitTypes.ESCAPE_ORBIT:
-            message += escapeOrbitString;
-            break;
-          default:
-            break;
-        }
-
-        warningText.setString( message );
-
-        //REVIEW: This would be a separate link to keep things centered
-        warningText.center = center;
+        return StringUtils.fillIn( warningString, {
+          message: orbitType === OrbitTypes.CRASH_ORBIT ? crashOrbitString :
+                    orbitType === OrbitTypes.ESCAPE_ORBIT ? escapeOrbitString : ''
+        } );
       }
     );
 
+    const warningText = new RichText( messageProperty, combineOptions<TextOptions>( {
+      maxWidth: 500
+    }, SolarSystemCommonConstants.TITLE_OPTIONS ) );
+
+    messageProperty.link( () => {
+      warningText.center = center;
+    } );
+
     super( {
       isDisposable: false,
       visibleProperty: DerivedProperty.not( allowedOrbitProperty ),

diff --git a/keplers-laws-strings_en.json b/keplers-laws-strings_en.json
@@ -140,8 +140,8 @@
   "pattern.valueUnits": {
     "value": "{{value}} {{units}}"
   },
-  "warning.warning": {
-    "value": "Warning"
+  "warning.warningPattern": {
+    "value": "Warning: {{message}}"
   },
   "warning.crashOrbit": {
     "value": "The body will crash into the sun"