Refactor WordAccordionBox, TotalAccordionBox, and TenFrameAccordionBo…

…x to extend NumberPlayAccordionBox on Ten+Twenty screens, improve their content sizing and spacing, see #143
phetsims · Feb 9, 2022 · 6bb3dde · 6bb3dde
1 parent 7b1420b
commit 6bb3dde
Show file tree

Hide file tree

Showing 7 changed files with 80 additions and 108 deletions.
diff --git a/js/common/NumberPlayConstants.ts b/js/common/NumberPlayConstants.ts
@@ -95,11 +95,11 @@ const NumberPlayConstants = {
   // accordion box sizing for the 'Ten', 'Twenty', and 'Compare' screens
 
   // sizing for the 'upper' accordion boxes, which include the 'Number', 'Total', and 'Ten Frame' accordion boxes
-  TOTAL_ACCORDION_BOX_WIDTH: 200,           // width of the 'Total' accordion box on all three screens
-  UPPER_OUTER_ACCORDION_BOX_WIDTH: 310,     // width of the accordion boxes in the upper left and upper right of the
+  TOTAL_ACCORDION_BOX_WIDTH: 199,           // width of the 'Total' accordion box on all three screens
+  UPPER_OUTER_ACCORDION_BOX_WIDTH: 308,     // width of the accordion boxes in the upper left and upper right of the
                                             // 'Ten' and 'Twenty' screens
   UPPER_OUTER_AB_TITLE_MAX_WIDTH: 254,      // max width of the title of the upper outer accordion boxes
-  TEN_UPPER_ACCORDION_BOX_HEIGHT: 146,      // height of the upper accordion boxes on the 'Ten' screen
+  TEN_UPPER_ACCORDION_BOX_HEIGHT: 145,      // height of the upper accordion boxes on the 'Ten' screen
   TWENTY_UPPER_ACCORDION_BOX_HEIGHT: 98,    // height of the upper accordion boxes on the 'Twenty' screen
 
   // sizing for the 'lower' accordion boxes, which include the 'Ones' and 'Objects' accordion boxes

diff --git a/js/common/view/NumberPlayScreenView.ts b/js/common/view/NumberPlayScreenView.ts
@@ -17,10 +17,10 @@ import groupingScene2_png from '../../../images/groupingScene2_png.js';
 import groupingScene3_png from '../../../images/groupingScene3_png.js';
 import numberPlay from '../../numberPlay.js';
 import NumberPlayColors from '../NumberPlayColors.js';
-import NumberPlayConstants, { AccordionBoxOptions } from '../NumberPlayConstants.js';
+import NumberPlayConstants from '../NumberPlayConstants.js';
 import CountingAccordionBox from './CountingAccordionBox.js';
 import SpeechSynthesisButton from './SpeechSynthesisButton.js';
-import TenFrameAccordionBox from './TenFrameAccordionBox.js';
+import TenFrameAccordionBox, { TenFrameAccordionBoxOptions } from './TenFrameAccordionBox.js';
 import TotalAccordionBox, { TotalAccordionBoxOptions } from './TotalAccordionBox.js';
 import WordAccordionBox, { WordAccordionBoxOptions } from './WordAccordionBox.js';
 import NumberPlayModel from '../model/NumberPlayModel.js';
@@ -36,7 +36,7 @@ import SpeechSynthesisAnnouncer from '../../../../utterance-queue/js/SpeechSynth
 type NumberPlayScreenViewOptions = {
   wordAccordionBoxOptions: WordAccordionBoxOptions,
   totalAccordionBoxOptions: TotalAccordionBoxOptions,
-  tenFrameAccordionBoxOptions: Partial<AccordionBoxOptions>,
+  tenFrameAccordionBoxOptions: TenFrameAccordionBoxOptions,
   upperAccordionBoxHeight: number,
   lowerAccordionBoxHeight: number,
   tandem: Tandem
@@ -94,7 +94,7 @@ class NumberPlayScreenView extends ScreenView {
       model.currentNumberProperty,
       options.upperAccordionBoxHeight, merge( {
         expandedProperty: this.tenFrameAccordionBoxExpandedProperty
-      }, options.tenFrameAccordionBoxOptions ) );
+      }, options.tenFrameAccordionBoxOptions ) as TenFrameAccordionBoxOptions );
     tenFrameAccordionBox.right = this.layoutBounds.maxX - NumberPlayConstants.ACCORDION_BOX_MARGIN_X;
     tenFrameAccordionBox.top = wordAccordionBox.top;
     this.addChild( tenFrameAccordionBox );

diff --git a/js/common/view/TenFrameAccordionBox.ts b/js/common/view/TenFrameAccordionBox.ts
@@ -7,40 +7,36 @@
  * @author Chris Klusendorf (PhET Interactive Simulations)
  */
 
-import merge from '../../../../phet-core/js/merge.js';
-import { Rectangle, Text } from '../../../../scenery/js/imports.js';
-import AccordionBox from '../../../../sun/js/AccordionBox.js';
 import numberPlayStrings from '../../numberPlayStrings.js';
 import numberPlay from '../../numberPlay.js';
-import NumberPlayConstants, { AccordionBoxOptions } from '../NumberPlayConstants.js';
+import NumberPlayConstants from '../NumberPlayConstants.js';
 import TenFrameNode from './TenFrameNode.js';
 import NumberProperty from '../../../../axon/js/NumberProperty.js';
+import NumberPlayAccordionBox, { NumberPlayAccordionBoxOptions } from './NumberPlayAccordionBox.js';
+import optionize from '../../../../phet-core/js/optionize.js';
 
-// strings
-const tenFrameString = numberPlayStrings.tenFrame;
+// types
+type TenFrameAccordionBoxSelfOptions = {
+  tenFrameOffsetX: number
+};
+export type TenFrameAccordionBoxOptions = TenFrameAccordionBoxSelfOptions & NumberPlayAccordionBoxOptions;
 
-class TenFrameAccordionBox extends AccordionBox {
+class TenFrameAccordionBox extends NumberPlayAccordionBox {
 
-  constructor( currentNumberProperty: NumberProperty, height: number, provideOptions: Partial<AccordionBoxOptions> ) {
+  constructor( currentNumberProperty: NumberProperty, height: number, options: TenFrameAccordionBoxOptions ) {
 
-    const options = merge( {
-      titleNode: new Text( tenFrameString, {
-        font: NumberPlayConstants.ACCORDION_BOX_TITLE_FONT,
-        maxWidth: NumberPlayConstants.UPPER_OUTER_AB_TITLE_MAX_WIDTH
-      } ),
-      minWidth: NumberPlayConstants.UPPER_OUTER_ACCORDION_BOX_WIDTH,
-      maxWidth: NumberPlayConstants.UPPER_OUTER_ACCORDION_BOX_WIDTH
-    }, NumberPlayConstants.ACCORDION_BOX_OPTIONS, provideOptions ) as AccordionBoxOptions;
-
-    const contentNode = new Rectangle( { rectHeight: height } );
+    super( NumberPlayConstants.UPPER_OUTER_ACCORDION_BOX_WIDTH, height,
+      optionize<TenFrameAccordionBoxOptions, TenFrameAccordionBoxSelfOptions, NumberPlayAccordionBoxOptions>( {
+        titleString: numberPlayStrings.tenFrame,
+        titleMaxWidth: NumberPlayConstants.UPPER_OUTER_AB_TITLE_MAX_WIDTH
+      }, options ) );
 
     // create, scale, and add the TenFrameNode
     const tenFrameNode = new TenFrameNode( currentNumberProperty );
     tenFrameNode.scale( height / tenFrameNode.height / 2 );
-    tenFrameNode.centerY = contentNode.centerY;
-    contentNode.addChild( tenFrameNode );
-
-    super( contentNode, options );
+    tenFrameNode.centerX = this.contentBounds.centerX + options.tenFrameOffsetX;
+    tenFrameNode.centerY = this.contentBounds.centerY;
+    this.contentNode.addChild( tenFrameNode );
   }
 }
 

diff --git a/js/common/view/TotalAccordionBox.ts b/js/common/view/TotalAccordionBox.ts
@@ -7,45 +7,34 @@
  * @author Chris Klusendorf (PhET Interactive Simulations)
  */
 
-import merge from '../../../../phet-core/js/merge.js';
 import NumberDisplay from '../../../../scenery-phet/js/NumberDisplay.js';
-import { Font, HBox, Rectangle, Text, VBox } from '../../../../scenery/js/imports.js';
-import AccordionBox from '../../../../sun/js/AccordionBox.js';
+import { Font, HBox, VBox } from '../../../../scenery/js/imports.js';
 import ArrowButton from '../../../../sun/js/buttons/ArrowButton.js';
 import numberPlayStrings from '../../numberPlayStrings.js';
 import numberPlay from '../../numberPlay.js';
-import NumberPlayConstants, { AccordionBoxOptions } from '../NumberPlayConstants.js';
+import NumberPlayConstants from '../NumberPlayConstants.js';
 import NumberProperty from '../../../../axon/js/NumberProperty.js';
 import Range from '../../../../dot/js/Range.js';
+import NumberPlayAccordionBox, { NumberPlayAccordionBoxOptions } from './NumberPlayAccordionBox.js';
+import optionize from '../../../../phet-core/js/optionize.js';
 
 // types
-export type TotalAccordionBoxOptions = {
+type TotalAccordionBoxSelfOptions = {
   font: Font,
-  contentXMargin?: number,
   arrowButtonOptions: Object, // TODO-TS: should be ArrowButtonOptions
   arrowButtonSpacing: number
 };
-type TotalAccordionBoxImplementationOptions = AccordionBoxOptions & TotalAccordionBoxOptions;
+export type TotalAccordionBoxOptions = TotalAccordionBoxSelfOptions & NumberPlayAccordionBoxOptions;
 
-// strings
-const totalString = numberPlayStrings.total;
+class TotalAccordionBox extends NumberPlayAccordionBox {
 
-class TotalAccordionBox extends AccordionBox {
+  constructor( currentNumberProperty: NumberProperty, height: number, options: TotalAccordionBoxOptions ) {
 
-  constructor( currentNumberProperty: NumberProperty, height: number, providedOptions: TotalAccordionBoxOptions ) {
-
-    const options = merge( {
-      titleNode: new Text( totalString, {
-        font: NumberPlayConstants.ACCORDION_BOX_TITLE_FONT,
-        maxWidth: 142 // empirically determined to not shrink accordion box content
-      } ),
-      minWidth: NumberPlayConstants.TOTAL_ACCORDION_BOX_WIDTH,
-      maxWidth: NumberPlayConstants.TOTAL_ACCORDION_BOX_WIDTH
-    }, NumberPlayConstants.ACCORDION_BOX_OPTIONS, providedOptions ) as TotalAccordionBoxImplementationOptions;
-
-    const contentNode = new Rectangle( {
-      rectHeight: height
-    } );
+    super( NumberPlayConstants.TOTAL_ACCORDION_BOX_WIDTH, height,
+      optionize<TotalAccordionBoxOptions, TotalAccordionBoxSelfOptions, NumberPlayAccordionBoxOptions>( {
+        titleString: numberPlayStrings.total,
+        titleMaxWidth: 142
+      }, options ) );
 
     // create the NumberDisplay, which is a numerical representation of the current number. always format for numbers
     // up to twenty so the display looks consistent across screens.
@@ -81,10 +70,8 @@ class TotalAccordionBox extends AccordionBox {
 
     // arrange and add the number display and arrow buttons
     const numberControl = new HBox( { children: [ numberDisplay, arrowButtons ] } );
-    numberControl.center = contentNode.center;
-    contentNode.addChild( numberControl );
-
-    super( contentNode, options );
+    numberControl.center = this.contentBounds.center;
+    this.contentNode.addChild( numberControl );
   }
 }
 

diff --git a/js/common/view/WordAccordionBox.ts b/js/common/view/WordAccordionBox.ts
@@ -7,70 +7,61 @@
  * @author Chris Klusendorf (PhET Interactive Simulations)
  */
 
-import merge from '../../../../phet-core/js/merge.js';
-import { Font, Rectangle, Text } from '../../../../scenery/js/imports.js';
-import AccordionBox from '../../../../sun/js/AccordionBox.js';
+import { Font, Text } from '../../../../scenery/js/imports.js';
 import numberPlay from '../../numberPlay.js';
 import numberPlayStrings from '../../numberPlayStrings.js';
-import NumberPlayConstants, { AccordionBoxOptions } from '../NumberPlayConstants.js';
+import NumberPlayConstants from '../NumberPlayConstants.js';
 import LocaleSwitch from './LocaleSwitch.js';
 import NumberProperty from '../../../../axon/js/NumberProperty.js';
 import Vector2 from '../../../../dot/js/Vector2.js';
 import BooleanProperty from '../../../../axon/js/BooleanProperty.js';
 import Property from '../../../../axon/js/Property.js';
+import NumberPlayAccordionBox, { NumberPlayAccordionBoxOptions } from './NumberPlayAccordionBox.js';
+import optionize from '../../../../phet-core/js/optionize.js';
 
 // types
-export type WordAccordionBoxOptions = {
+type WordAccordionBoxSelfOptions = {
   textOffset: Vector2,
-  localeSwitchOffset: Vector2,
+  localeSwitchOffsetY: number,
   font: Font
 };
-type WordAccordionBoxImplementationOptions = AccordionBoxOptions & WordAccordionBoxOptions;
+export type WordAccordionBoxOptions = WordAccordionBoxSelfOptions & NumberPlayAccordionBoxOptions;
 
 // constants
-const CONTENT_MAX_WIDTH = 260; // empirically determined to not shrink the accordion box content
+const TEXT_MARGIN = 5;
 
-// strings
-const wordString = numberPlayStrings.word;
 
-class WordAccordionBox extends AccordionBox {
+class WordAccordionBox extends NumberPlayAccordionBox {
 
   constructor( currentNumberProperty: NumberProperty, isPrimaryLocaleProperty: BooleanProperty, height: number,
-               providedOptions: WordAccordionBoxOptions ) {
+               options: WordAccordionBoxOptions ) {
 
-    const options = merge( {
-      titleNode: new Text( wordString, {
-        font: NumberPlayConstants.ACCORDION_BOX_TITLE_FONT,
-        maxWidth: NumberPlayConstants.UPPER_OUTER_AB_TITLE_MAX_WIDTH
-      } ),
-      minWidth: NumberPlayConstants.UPPER_OUTER_ACCORDION_BOX_WIDTH,
-      maxWidth: NumberPlayConstants.UPPER_OUTER_ACCORDION_BOX_WIDTH
-    }, NumberPlayConstants.ACCORDION_BOX_OPTIONS, providedOptions ) as WordAccordionBoxImplementationOptions;
+    super( NumberPlayConstants.UPPER_OUTER_ACCORDION_BOX_WIDTH, height,
+      optionize<WordAccordionBoxOptions, WordAccordionBoxSelfOptions, NumberPlayAccordionBoxOptions>( {
+        titleString: numberPlayStrings.word,
+        titleMaxWidth: NumberPlayConstants.UPPER_OUTER_AB_TITLE_MAX_WIDTH
+      }, options ) );
 
-    const contentNode = new Rectangle( {
-      rectHeight: height,
-      rectWidth: CONTENT_MAX_WIDTH
-    } );
+    const showLocaleSwitch = !!phet.numberPlay.secondLocaleStrings;
 
+    const wordTextCenterY = showLocaleSwitch ? this.contentBounds.centerY + options.textOffset.y : this.contentBounds.centerY;
     const wordText = new Text( NumberPlayConstants.NUMBER_TO_STRING[ currentNumberProperty.value ], {
       font: options.font,
-      maxWidth: CONTENT_MAX_WIDTH - options.textOffset.x
+      maxWidth: this.contentBounds.width - options.textOffset.x - TEXT_MARGIN
     } );
-    wordText.left = contentNode.left + options.textOffset.x;
-    wordText.centerY = contentNode.centerY + options.textOffset.y;
-    contentNode.addChild( wordText );
+    wordText.left = this.contentBounds.left + options.textOffset.x;
+    wordText.centerY = wordTextCenterY;
+    this.contentNode.addChild( wordText );
 
-    // make sure the offset doesn't cause the LocaleSwitch to poke out of either end of the content node when at its max width
-    const localeSwitchMaxWidth = CONTENT_MAX_WIDTH - Math.abs( options.localeSwitchOffset.x ) * 2;
+    // used make sure the localeSwitch doesn't expand outside of this accordion box
+    const localeSwitchMaxWidth = this.contentBounds.width - TEXT_MARGIN * 2;
 
     // create and add the LocaleSwitch
     const localeSwitch = new LocaleSwitch( isPrimaryLocaleProperty, localeSwitchMaxWidth );
-    localeSwitch.centerX = contentNode.centerX + options.localeSwitchOffset.x;
-    localeSwitch.bottom = contentNode.bottom + options.localeSwitchOffset.y;
-    localeSwitch.visible = !!phet.numberPlay.secondLocaleStrings;
-    contentNode.addChild( localeSwitch );
-
-    super( contentNode, options );
+    localeSwitch.centerX = this.contentBounds.centerX;
+    localeSwitch.bottom = this.contentNode.bottom + options.localeSwitchOffsetY;
+    localeSwitch.visible = showLocaleSwitch;
+    this.contentNode.addChild( localeSwitch );
 
     // update the word if the current number or locale changes
     Property.lazyMultilink( [ currentNumberProperty, isPrimaryLocaleProperty ],

diff --git a/js/ten/TenScreen.ts b/js/ten/TenScreen.ts
@@ -14,7 +14,7 @@ import { Image } from '../../../scenery/js/imports.js';
 import tenScreenIcon_png from '../../images/tenScreenIcon_png.js';
 import NumberPlayModel from '../common/model/NumberPlayModel.js';
 import NumberPlayColors from '../common/NumberPlayColors.js';
-import NumberPlayConstants, { AccordionBoxOptions } from '../common/NumberPlayConstants.js';
+import NumberPlayConstants from '../common/NumberPlayConstants.js';
 import NumberPlayScreenView from '../common/view/NumberPlayScreenView.js';
 import numberPlay from '../numberPlay.js';
 import numberPlayStrings from '../numberPlayStrings.js';
@@ -40,13 +40,12 @@ class TenScreen extends Screen {
       wordAccordionBoxOptions: {
         fill: NumberPlayColors.purpleBackgroundColorProperty,
         font: new PhetFont( 62 ),
-        textOffset: new Vector2( 0, -1 ),
-        localeSwitchOffset: new Vector2( 0, -12 )
+        textOffset: new Vector2( 30, -1 ),
+        localeSwitchOffsetY: -12
       },
       totalAccordionBoxOptions: {
         fill: NumberPlayColors.lightOrangeBackgroundColorProperty,
         font: new PhetFont( 98 ),
-        contentXMargin: 0, // zero out to manage x margins in subclass
         arrowButtonOptions: {
           arrowWidth: 20, // empirically determined
           arrowHeight: 20 // empirically determined
@@ -55,8 +54,8 @@ class TenScreen extends Screen {
       },
       tenFrameAccordionBoxOptions: {
         fill: NumberPlayColors.purpleBackgroundColorProperty,
-        contentAlign: 'center'
-      } as AccordionBoxOptions,
+        tenFrameOffsetX: 0
+      },
       upperAccordionBoxHeight: NumberPlayConstants.TEN_UPPER_ACCORDION_BOX_HEIGHT,
       lowerAccordionBoxHeight: NumberPlayConstants.TEN_LOWER_ACCORDION_BOX_HEIGHT,
       tandem: tandem.createTandem( 'view' )

diff --git a/js/twenty/TwentyScreen.ts b/js/twenty/TwentyScreen.ts
@@ -15,7 +15,7 @@ import Tandem from '../../../tandem/js/Tandem.js';
 import twentyScreenIcon_png from '../../images/twentyScreenIcon_png.js';
 import NumberPlayModel from '../common/model/NumberPlayModel.js';
 import NumberPlayColors from '../common/NumberPlayColors.js';
-import NumberPlayConstants, { AccordionBoxOptions } from '../common/NumberPlayConstants.js';
+import NumberPlayConstants from '../common/NumberPlayConstants.js';
 import NumberPlayScreenView from '../common/view/NumberPlayScreenView.js';
 import numberPlay from '../numberPlay.js';
 import numberPlayStrings from '../numberPlayStrings.js';
@@ -41,23 +41,22 @@ class TwentyScreen extends Screen {
       wordAccordionBoxOptions: {
         fill: NumberPlayColors.orangeBackgroundColorProperty,
         font: new PhetFont( 54 ),
-        contentXMargin: 10, // zero out to manage x margins in subclass TODO: unsure why 10 is needed to act like 0
-        textOffset: new Vector2( 10, -10 ),
-        localeSwitchOffset: new Vector2( -10, -7 )
+        textOffset: new Vector2( 40, -10 ),
+        localeSwitchOffsetY: -7
       },
       totalAccordionBoxOptions: {
         fill: NumberPlayColors.lightPurpleBackgroundColorProperty,
-        font: new PhetFont( 76 ),
+        font: new PhetFont( 80 ),
         arrowButtonOptions: {
-          arrowWidth: 15, // empirically determined
-          arrowHeight: 15 // empirically determined
+          arrowWidth: 16, // empirically determined
+          arrowHeight: 16 // empirically determined
         },
         arrowButtonSpacing: 5 // empirically determined
       },
       tenFrameAccordionBoxOptions: {
         fill: NumberPlayColors.orangeBackgroundColorProperty,
-        contentAlign: 'right'
-      } as AccordionBoxOptions,
+        tenFrameOffsetX: 13
+      },
       upperAccordionBoxHeight: NumberPlayConstants.TWENTY_UPPER_ACCORDION_BOX_HEIGHT,
       lowerAccordionBoxHeight: NumberPlayConstants.TWENTY_LOWER_ACCORDION_BOX_HEIGHT,
       tandem: tandem.createTandem( 'view' )