Skip to content

Commit

Permalink
REVIEW notes, see phetsims/my-solar-system#88
Browse files Browse the repository at this point in the history
  • Loading branch information
jonathanolson committed Mar 9, 2023
1 parent 112aa50 commit b57f45f
Showing 1 changed file with 8 additions and 0 deletions.
8 changes: 8 additions & 0 deletions js/keplers-laws/view/OrbitalWarningMessage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@ export default class OrbitalWarningMessage extends Node {
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, SolarSystemCommonConstants.TITLE_OPTIONS );

Expand All @@ -38,6 +42,8 @@ export default class OrbitalWarningMessage extends Node {
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:
Expand All @@ -51,6 +57,8 @@ export default class OrbitalWarningMessage extends Node {
}

warningText.setString( message );

//REVIEW: This would be a separate link to keep things centered
warningText.center = options.center;
}
);
Expand Down

0 comments on commit b57f45f

Please sign in to comment.