samreid
commented
1 year ago More ideas in this patch:
```diff
Subject: [PATCH] Update TODOs, see https://github.com/phetsims/center-and-variability/issues/45
---
Index: main/center-and-variability/js/mean-and-median/view/MeanAndMedianScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/center-and-variability/js/mean-and-median/view/MeanAndMedianScreenView.ts b/main/center-and-variability/js/mean-and-median/view/MeanAndMedianScreenView.ts
--- a/main/center-and-variability/js/mean-and-median/view/MeanAndMedianScreenView.ts (revision c090d08e0531b8f980cb38f51954317cdcfad013)
+++ b/main/center-and-variability/js/mean-and-median/view/MeanAndMedianScreenView.ts (date 1683210576999)
@@ -37,7 +37,10 @@
super( model, options );
- this.setAccordionBoxWithAlignedContent( new MeanAndMedianAccordionBox( model, this.layoutBounds, options.tandem.createTandem( 'accordionBox' ), this.questionBar.bottom + CAVConstants.SCREEN_VIEW_Y_MARGIN, this.playAreaNumberLineNode ) );
+ this.setAccordionBoxWithAlignedContent(
+ new MeanAndMedianAccordionBox( model, this.layoutBounds, options.tandem.createTandem( 'accordionBox' ), this.questionBar.bottom + CAVConstants.SCREEN_VIEW_Y_MARGIN, this.playAreaNumberLineNode ),
+ this.plotNode
+ );
const iconGroup = new AlignGroup();
super.setBottomCheckboxGroup( [
Index: main/center-and-variability/js/mean-and-median/view/MeanAndMedianAccordionBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/center-and-variability/js/mean-and-median/view/MeanAndMedianAccordionBox.ts b/main/center-and-variability/js/mean-and-median/view/MeanAndMedianAccordionBox.ts
--- a/main/center-and-variability/js/mean-and-median/view/MeanAndMedianAccordionBox.ts (revision c090d08e0531b8f980cb38f51954317cdcfad013)
+++ b/main/center-and-variability/js/mean-and-median/view/MeanAndMedianAccordionBox.ts (date 1683206737157)
@@ -13,6 +13,7 @@
import VerticalCheckboxGroup from '../../../../sun/js/VerticalCheckboxGroup.js';
export default class MeanAndMedianAccordionBox extends CAVAccordionBox {
+ public readonly plotNode: MedianPlotNode;
public constructor( model: MeanAndMedianModel, layoutBounds: Bounds2, tandem: Tandem, top: number, playAreaNumberLineNode: Node ) {
const iconGroup = new AlignGroup();
@@ -35,14 +36,15 @@
maxWidth: 300
} ),
layoutBounds,
- checkboxGroup,
- {
+ checkboxGroup, {
leftMargin: 0,
tandem: tandem,
top: top,
centerX: layoutBounds.centerX
}
);
+
+ this.plotNode = plotNode;
}
}
Index: main/center-and-variability/js/variability/view/VariabilityPlotNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/center-and-variability/js/variability/view/VariabilityPlotNode.ts b/main/center-and-variability/js/variability/view/VariabilityPlotNode.ts
--- a/main/center-and-variability/js/variability/view/VariabilityPlotNode.ts (revision c090d08e0531b8f980cb38f51954317cdcfad013)
+++ b/main/center-and-variability/js/variability/view/VariabilityPlotNode.ts (date 1683211207001)
@@ -33,21 +33,24 @@
} ),
tandemName: 'rangeNode',
value: VariabilityMeasure.RANGE
- }, {
- createNode: tandem => new IQRNode( model, sceneModel, {
- parentContext: 'accordion',
- tandem: tandem.createTandem( 'iqrNode' )
- } ),
- tandemName: 'iqrNode',
- value: VariabilityMeasure.IQR
- }, {
- createNode: tandem => new MADNode( model, sceneModel, {
- parentContext: 'accordion',
- tandem: tandem.createTandem( 'madNode' )
- } ),
- tandemName: 'madNode',
- value: VariabilityMeasure.MAD
- } ], {
+ }
+ // , {
+ // createNode: tandem => new IQRNode( model, sceneModel, {
+ // parentContext: 'accordion',
+ // tandem: tandem.createTandem( 'iqrNode' )
+ // } ),
+ // tandemName: 'iqrNode',
+ // value: VariabilityMeasure.IQR
+ // }
+ , {
+ createNode: tandem => new MADNode( model, sceneModel, {
+ parentContext: 'accordion',
+ tandem: tandem.createTandem( 'madNode' )
+ } ),
+ tandemName: 'madNode',
+ value: VariabilityMeasure.MAD
+ }
+ ], {
tandem: providedOptions.tandem.createTandem( 'toggleNode' )
} );
this.addChild( toggleNode );
Index: main/center-and-variability/js/variability/view/VariabilityAccordionBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/center-and-variability/js/variability/view/VariabilityAccordionBox.ts b/main/center-and-variability/js/variability/view/VariabilityAccordionBox.ts
--- a/main/center-and-variability/js/variability/view/VariabilityAccordionBox.ts (revision c090d08e0531b8f980cb38f51954317cdcfad013)
+++ b/main/center-and-variability/js/variability/view/VariabilityAccordionBox.ts (date 1683211751744)
@@ -1,7 +1,7 @@
// Copyright 2023, University of Colorado Boulder
import CAVAccordionBox from '../../common/view/CAVAccordionBox.js';
-import { AlignGroup, Text } from '../../../../scenery/js/imports.js';
+import { AlignGroup, Color, Node, Rectangle, Text } from '../../../../scenery/js/imports.js';
import CenterAndVariabilityStrings from '../../CenterAndVariabilityStrings.js';
import PhetFont from '../../../../scenery-phet/js/PhetFont.js';
import Bounds2 from '../../../../dot/js/Bounds2.js';
@@ -11,7 +11,6 @@
import VariabilityPlotNode from './VariabilityPlotNode.js';
import InfoButton from '../../../../scenery-phet/js/buttons/InfoButton.js';
import VariabilityMeasure from '../model/VariabilityMeasure.js';
-import CAVConstants from '../../common/CAVConstants.js';
import DerivedProperty from '../../../../axon/js/DerivedProperty.js';
import DynamicProperty from '../../../../axon/js/DynamicProperty.js';
import ToggleNode from '../../../../sun/js/ToggleNode.js';
@@ -19,6 +18,7 @@
import TopRepresentationCheckboxGroup from '../../common/view/TopRepresentationCheckboxGroup.js';
export default class VariabilityAccordionBox extends CAVAccordionBox {
+ public readonly plotNode: ToggleNode;
public constructor( model: VariabilityModel, layoutBounds: Bounds2, tandem: Tandem, top: number ) {
@@ -39,24 +39,10 @@
};
} );
- const accordionBoxContents = new ToggleNode( model.selectedSceneModelProperty, contents );
-
- const infoButton = new InfoButton( {
- iconFill: 'cornflowerblue',
- scale: 0.5,
- touchAreaDilation: 5,
- tandem: tandem.createTandem( 'infoButton' ),
- listener: () => {
- model.isInfoShowingProperty.value = true;
- },
-
- // TODO: How to position this properly? Can we use AlignBox?
- top: 20,
- right: accordionBoxContents.right - 10
- } );
- accordionBoxContents.addChild( infoButton );
-
const iconGroup = new AlignGroup();
+
+ const RECT_WIDTH = 900;
+ const RECT_HEIGHT = 150;
const checkboxToggleNode = new ToggleNode( model.selectedVariabilityProperty, [
{
createNode: tandem => new VerticalCheckboxGroup( [
@@ -79,7 +65,37 @@
tandemName: 'madAccordionCheckboxGroup',
value: VariabilityMeasure.MAD
}
- ] );
+ ], {
+ right: RECT_WIDTH - 10,
+ centerY: RECT_HEIGHT / 2
+ } );
+
+ const plotNodes = new ToggleNode( model.selectedSceneModelProperty, contents, {
+ alignChildren: ToggleNode.NONE,
+ // excludeInvisibleChildrenFromBounds: true
+ // left: 0,
+ // top: 0
+ } );
+
+ const accordionBoxContents = new Rectangle( 0, 0, RECT_WIDTH, RECT_HEIGHT, {
+ fill: new Color( 0, 0, 255, 0.1 ),
+ children: [
+ // plotNodes,
+ new InfoButton( {
+ iconFill: 'cornflowerblue',
+ scale: 0.5,
+ touchAreaDilation: 5,
+ tandem: tandem.createTandem( 'infoButton' ),
+ listener: () => {
+ model.isInfoShowingProperty.value = true;
+ },
+
+ top: 0,
+ right: RECT_WIDTH - 10
+ } ),
+ checkboxToggleNode
+ ]
+ } );
super( model.resetEmitter, accordionBoxContents,
new Text( accordionBoxTitleProperty, {
@@ -87,13 +103,19 @@
maxWidth: 300
} ),
layoutBounds,
- checkboxToggleNode,
+ // checkboxToggleNode,
{
- leftMargin: 70,
+ // leftMargin: 70,
tandem: tandem,
top: top,
- right: layoutBounds.right - CAVConstants.SCREEN_VIEW_X_MARGIN
+ right: layoutBounds.right
} );
+
+ // ManualConstraint.create( this, [ infoButton, this.expandedTitleBar ], ( infoButtonWrapper, expandedTitleBarWrapper ) => {
+ // infoButtonWrapper.rightTop = expandedTitleBarWrapper.rightTop;
+ // } );
+
+ this.plotNode = plotNodes;
}
}
Index: main/center-and-variability/js/common/view/CAVAccordionBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/center-and-variability/js/common/view/CAVAccordionBox.ts b/main/center-and-variability/js/common/view/CAVAccordionBox.ts
--- a/main/center-and-variability/js/common/view/CAVAccordionBox.ts (revision c090d08e0531b8f980cb38f51954317cdcfad013)
+++ b/main/center-and-variability/js/common/view/CAVAccordionBox.ts (date 1683207764659)
@@ -9,17 +9,15 @@
import Bounds2 from '../../../../dot/js/Bounds2.js';
import StrictOmit from '../../../../phet-core/js/types/StrictOmit.js';
-import { AlignBox, Node, Rectangle } from '../../../../scenery/js/imports.js';
+import { Node } from '../../../../scenery/js/imports.js';
import AccordionBox, { AccordionBoxOptions } from '../../../../sun/js/AccordionBox.js';
-import CAVConstants from '../CAVConstants.js';
import optionize from '../../../../phet-core/js/optionize.js';
import centerAndVariability from '../../centerAndVariability.js';
-import { Shape } from '../../../../kite/js/imports.js';
import PickRequired from '../../../../phet-core/js/types/PickRequired.js';
import TEmitter from '../../../../axon/js/TEmitter.js';
type SelfOptions = {
- leftMargin: number;
+ // leftMargin: number;
};
export type CAVAccordionBoxOptions =
SelfOptions
@@ -32,11 +30,11 @@
export default class CAVAccordionBox extends AccordionBox {
- public readonly plotNode: Node;
+ // public readonly plotNode: Node;
// NOTE: The positions of the passed-in nodes are modified directly, so they cannot be used in the scenery DAG
- public constructor( resetEmitter: TEmitter, plotNode: Node, titleNode: Node,
- layoutBounds: Bounds2, checkboxGroup: Node, providedOptions: CAVAccordionBoxOptions ) {
+ public constructor( resetEmitter: TEmitter, contentNode: Node, titleNode: Node,
+ layoutBounds: Bounds2, providedOptions: CAVAccordionBoxOptions ) {
const options = optionize()( {
titleAlignX: 'left',
@@ -59,40 +57,43 @@
titleNode: titleNode
}, providedOptions );
- const backgroundNode = new Rectangle( {
- rectHeight: 140,
- rectWidth: layoutBounds.width - CAVConstants.SCREEN_VIEW_X_MARGIN * 2 - CONTENT_MARGIN * 2 - options.leftMargin
- } );
+ // const backgroundNode = new Rectangle( {
+ // rectHeight: 140,
+ // rectWidth: layoutBounds.width - CAVConstants.SCREEN_VIEW_X_MARGIN * 2 - CONTENT_MARGIN * 2 - options.leftMargin
+ // } );
// Since the title is visible while the accordion box is open, this background will not any area above the bottom of
// the expand/collapse button. To vertically-center things, make a new set of bounds that includes the missing space.
// Values come from the height of the expand/collapse button plus the y margin above and below it. Also add the
// horizontal content margin that is not part of backgroundNode so these bounds are the full area of the accordion box.
- const fullBackgroundBounds =
- backgroundNode.localBounds.withOffsets( CONTENT_MARGIN, CONTENT_MARGIN * 2 + BUTTON_SIDE_LENGTH, CONTENT_MARGIN, 0 );
-
- // add clip area so dot stacks that are taller than the accordion box are clipped appropriately
- backgroundNode.clipArea = Shape.bounds( fullBackgroundBounds );
+ // const fullBackgroundBounds =
+ // backgroundNode.localBounds.withOffsets( CONTENT_MARGIN, CONTENT_MARGIN * 2 + BUTTON_SIDE_LENGTH, CONTENT_MARGIN, 0 );
+ //
+ // // add clip area so dot stacks that are taller than the accordion box are clipped appropriately
+ // backgroundNode.clipArea = Shape.bounds( fullBackgroundBounds );
// Vertical positioning
- plotNode.centerY = fullBackgroundBounds.centerY;
- if ( plotNode.bottom > fullBackgroundBounds.bottom - 5 ) {
- plotNode.bottom = fullBackgroundBounds.bottom - 5;
- }
- backgroundNode.addChild( plotNode );
+ // contentNode.centerY = fullBackgroundBounds.centerY;
+ // if ( contentNode.bottom > fullBackgroundBounds.bottom - 5 ) {
+ // contentNode.bottom = fullBackgroundBounds.bottom - 5;
+ // }
+ // backgroundNode.addChild( contentNode );
- const checkboxAlignBox = new AlignBox( checkboxGroup,
- { xAlign: 'right', yAlign: 'center', rightMargin: 20, alignBounds: fullBackgroundBounds } );
+ // const checkboxAlignBox = new AlignBox( checkboxGroup, {
+ // xAlign: 'right',
+ // yAlign: 'center',
+ // rightMargin: 20
+ // alignBounds: fullBackgroundBounds
+ // } );
- backgroundNode.addChild( checkboxAlignBox );
+ // backgroundNode.addChild( checkboxAlignBox );
- super( backgroundNode, options );
+ super( contentNode, options );
resetEmitter.addListener( () => this.reset() );
- this.plotNode = plotNode;
+ this.plotNode = contentNode;
}
-
}
centerAndVariability.register( 'CAVAccordionBox', CAVAccordionBox );
\ No newline at end of file
Index: main/center-and-variability/js/common/view/CAVScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/center-and-variability/js/common/view/CAVScreenView.ts b/main/center-and-variability/js/common/view/CAVScreenView.ts
--- a/main/center-and-variability/js/common/view/CAVScreenView.ts (revision c090d08e0531b8f980cb38f51954317cdcfad013)
+++ b/main/center-and-variability/js/common/view/CAVScreenView.ts (date 1683210635625)
@@ -31,6 +31,9 @@
import SceneView from './SceneView.js';
import KickButtonGroup from './KickButtonGroup.js';
import DynamicProperty from '../../../../axon/js/DynamicProperty.js';
+import CAVPlotNode from './CAVPlotNode.js';
+import MeanAndMedianAccordionBox from '../../mean-and-median/view/MeanAndMedianAccordionBox.js';
+import VariabilityAccordionBox from '../../variability/view/VariabilityAccordionBox.js';
type SelfOptions = {
questionBarOptions: QuestionBarOptions;
@@ -189,12 +192,12 @@
* -Same offset and scale
* Given those assumptions, this code moves the dot plot so that its number line matches the play area one.
*/
- protected setAccordionBoxWithAlignedContent( accordionBox: CAVAccordionBox ): void {
+ protected setAccordionBoxWithAlignedContent( accordionBox: MeanAndMedianAccordionBox | VariabilityAccordionBox ): void {
this.setAccordionBox( accordionBox );
- ManualConstraint.create( this, [ this.playAreaNumberLineNode, accordionBox.plotNode ],
- ( lowerNumberLineWrapper, contentsWrapper ) => {
- contentsWrapper.x = lowerNumberLineWrapper.x;
- } );
+ // ManualConstraint.create( this, [ this.playAreaNumberLineNode, accordionBox.plotNode ],
+ // ( playAreaNumberLine, plotNode ) => {
+ // // plotNode.x = playAreaNumberLine.x;
+ // } );
}
protected setBottomCheckboxGroup( items: VerticalCheckboxGroupItem[] ): void {
Index: main/sun/js/ToggleNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/sun/js/ToggleNode.ts b/main/sun/js/ToggleNode.ts
--- a/main/sun/js/ToggleNode.ts (revision 8346e6d4343725b09c3556e51b66efc658e2a6a1)
+++ b/main/sun/js/ToggleNode.ts (date 1683204944669)
@@ -34,15 +34,6 @@
public constructor( valueProperty: TReadOnlyProperty, elements: ToggleNodeElement[], providedOptions?: ToggleNodeOptions ) {
- // assert && assert( Array.isArray( elements ), 'elements should be an array' );
- // if ( assert ) {
- // elements.forEach( element => {
- // const keys = _.keys( element );
- // assert && assert( keys.length === 2, 'each element should have two keys' );
- // assert && assert( keys[ 0 ] === 'value' || keys[ 1 ] === 'value', 'element should have a value key' );
- // } );
- // }
-
const options = optionize()( {
// SelfOptions
Index: main/center-and-variability/js/variability/view/VariabilityScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/center-and-variability/js/variability/view/VariabilityScreenView.ts b/main/center-and-variability/js/variability/view/VariabilityScreenView.ts
--- a/main/center-and-variability/js/variability/view/VariabilityScreenView.ts (revision c090d08e0531b8f980cb38f51954317cdcfad013)
+++ b/main/center-and-variability/js/variability/view/VariabilityScreenView.ts (date 1683211773992)
@@ -45,7 +45,17 @@
super( model, options );
- this.setAccordionBoxWithAlignedContent( new VariabilityAccordionBox( model, this.layoutBounds, options.tandem.createTandem( 'accordionBox' ), this.questionBar.bottom + CAVConstants.SCREEN_VIEW_Y_MARGIN ) );
+ const variabilityAccordionBox = new VariabilityAccordionBox( model, this.layoutBounds, options.tandem.createTandem( 'accordionBox' ), this.questionBar.bottom + CAVConstants.SCREEN_VIEW_Y_MARGIN );
+ this.setAccordionBox( variabilityAccordionBox );
+
+ // variabilityAccordionBox.plotNode.x = this.playAreaNumberLineNode.x;
+ this.addChild( variabilityAccordionBox.plotNode );
+
+ ManualConstraint.create( this, [ this.playAreaNumberLineNode, variabilityAccordionBox.plotNode, variabilityAccordionBox ],
+ ( playAreaNumberLine, plotNode, variabilityAccordionBox ) => {
+ plotNode.x = playAreaNumberLine.x;
+ plotNode.bottom = variabilityAccordionBox.bottom;
+ } );
ManualConstraint.create( this, [ variabilityMeasureRadioButtonGroup, this.accordionBox! ],
( variabilityRadioButtonGroupWrapper, accordionBoxWrapper ) => {
Index: main/sun/js/AccordionBox.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/main/sun/js/AccordionBox.ts b/main/sun/js/AccordionBox.ts
--- a/main/sun/js/AccordionBox.ts (revision 8346e6d4343725b09c3556e51b66efc658e2a6a1)
+++ b/main/sun/js/AccordionBox.ts (date 1683206150982)
@@ -105,7 +105,7 @@
private readonly expandedBox: Rectangle;
private readonly collapsedBox: Rectangle;
private readonly workaroundBox: Rectangle;
- private readonly expandedTitleBar: InteractiveHighlightPath;
+ protected readonly expandedTitleBar: InteractiveHighlightPath;
private readonly collapsedTitleBar: InteractiveHighlightRectangle;
private readonly containerNode: Node;
private readonly resetAccordionBox: () => void;
```
marlitas
jonathanolson
There are a lot of hacks and workarounds in the accordion box. I tried to sort it out in this version, but the ManualConstraint that aligns the plot nodes is throwing things off: