Skip to content

Commit

Permalink
REVIEW notes, see #88
Browse files Browse the repository at this point in the history
  • Loading branch information
jonathanolson committed Mar 9, 2023
1 parent 5ed6f89 commit 839d77a
Showing 1 changed file with 6 additions and 0 deletions.
6 changes: 6 additions & 0 deletions js/common/view/MySolarSystemScreenView.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ type SelfOptions = EmptySelfOptions;

export type IntroLabScreenViewOptions = SelfOptions & ScreenViewOptions;

//REVIEW: This is used once. Please inline it!
const spinnerOptions: NumberSpinnerOptions = {
deltaValue: 1,
touchAreaXDilation: 20,
Expand Down Expand Up @@ -89,6 +90,7 @@ export default class MySolarSystemScreenView extends SolarSystemCommonScreenView

const forceVectorSynchronizer = new ViewSynchronizer( this.componentsLayer, ( body: Body ) => {
return new VectorNode(
//REVIEW: Since this is spilling to a second line, it's easier to read if we put each option on its own separate line.
body, this.modelViewTransformProperty, model.gravityVisibleProperty, body.forceProperty,
0.05, { fill: PhetColorScheme.GRAVITATIONAL_FORCE }
);
Expand Down Expand Up @@ -136,6 +138,7 @@ export default class MySolarSystemScreenView extends SolarSystemCommonScreenView
} );

const topRightControlBox = new AlignBox(
//REVIEW: Braces are on separate lines unnecessarily, and it forces another indent level. Put ( { on same line as VBox.
new VBox(
{
spacing: 10,
Expand Down Expand Up @@ -172,6 +175,8 @@ export default class MySolarSystemScreenView extends SolarSystemCommonScreenView
accessibleName: MySolarSystemStrings.a11y.numberOfBodiesStringProperty
} ) )
],
//REVIEW: for phet-io, we'll not want to create this if it's not the lab screen. It would show up in the tandem
//REVIEW: structure.
visible: model.isLab,
spacing: 10
} );
Expand All @@ -195,6 +200,7 @@ export default class MySolarSystemScreenView extends SolarSystemCommonScreenView
children: [
new HBox( {
stretch: true,
//REVIEW: Issues with phet-io structure, see above (shouldn't even be created if it's not lab, no?)
visible: model.isLab,
children: [
new SolarSystemCommonCheckbox(
Expand Down

0 comments on commit 839d77a

Please sign in to comment.