From 03d318ae0791503dade1c59eea92b829fe220d85 Mon Sep 17 00:00:00 2001 From: Sam Reid Date: Wed, 26 Apr 2023 08:53:19 -0600 Subject: [PATCH] Address TODOs, see https://github.com/phetsims/center-and-variability/issues/45 --- js/common/CAVConstants.ts | 2 +- js/common/view/BottomRepresentationCheckboxGroup.ts | 2 -- js/common/view/CAVObjectNode.ts | 6 +++--- js/common/view/CAVPlotNode.ts | 4 +--- js/common/view/CardNode.ts | 2 +- js/common/view/CardNodeContainer.ts | 2 +- 6 files changed, 7 insertions(+), 11 deletions(-) diff --git a/js/common/CAVConstants.ts b/js/common/CAVConstants.ts index 746be4d6..5806b62a 100644 --- a/js/common/CAVConstants.ts +++ b/js/common/CAVConstants.ts @@ -45,7 +45,7 @@ const CAVConstants = { CHART_VIEW_WIDTH: ScreenView.DEFAULT_LAYOUT_BOUNDS.width - NUMBER_LINE_MARGIN_X * 2, NUMBER_LINE_MARGIN_X: NUMBER_LINE_MARGIN_X, - // TODO: This is the color from the design doc, but perhaps #777777 or darker would be better? Let's discuss once the IQR lines are drawn + // TODO-design: This is the color from the design doc, but perhaps #777777 or darker would be better? Let's discuss once the IQR lines are drawn GRAY_DATA_POINT_FILL: '#8f8f8f' }; diff --git a/js/common/view/BottomRepresentationCheckboxGroup.ts b/js/common/view/BottomRepresentationCheckboxGroup.ts index 030d001d..f2c1f15e 100644 --- a/js/common/view/BottomRepresentationCheckboxGroup.ts +++ b/js/common/view/BottomRepresentationCheckboxGroup.ts @@ -36,8 +36,6 @@ export default class BottomRepresentationCheckboxGroup extends VerticalCheckboxG spacing: 5, grow: 1, rows: [ [ - - // TODO: this will be odd to a11y because both buttons have the same text. Do we have alt text for the icons? Or maybe we need alt text for the entire checkbox? new Node( { children: [ text ], layoutOptions: { xAlign: 'left' } } ), iconGroup.createBox( icon, { layoutOptions: { xAlign: 'right' }, xAlign: 'center' } ) ] ] diff --git a/js/common/view/CAVObjectNode.ts b/js/common/view/CAVObjectNode.ts index 115c185a..d1931ddc 100644 --- a/js/common/view/CAVObjectNode.ts +++ b/js/common/view/CAVObjectNode.ts @@ -74,7 +74,7 @@ export default class CAVObjectNode extends Node { // TODO-UX: These should be edge to edge // TODO-UX: For small dots, there is an optical illusion or rasterizing/roundoff/aliasing issue that makes it // look lopsided (heavier on the left) - // TODO-DESIGN: This highlight is difficult to see + // TODO-DESIGN: This highlight is difficult to see. const medianHighlight = new Circle( viewRadius + 1.75, { fill: CAVColors.medianColorProperty } ); @@ -121,7 +121,7 @@ export default class CAVObjectNode extends Node { // Center the nested Node for compatibility with DragListener childNode.center = Vector2.ZERO; - // TODO: CK: Better comment that explains why this nested layer is necessary + // TODO: Add a comment that explains why this nested layer is necessary this.addChild( childNode ); this.addLinkedElement( cavObject, { @@ -226,7 +226,7 @@ export default class CAVObjectNode extends Node { Multilink.unmultilink( this.medianHighlightVisibleMultilink ); this.inputEnabledMultilink && Multilink.unmultilink( this.inputEnabledMultilink ); this.selfInputEnabledProperty && this.selfInputEnabledProperty.dispose(); - this.dragListener && this.hasInputListener( this.dragListener ) && this.removeInputListener( this.dragListener ); // TODO: is this needed? + this.dragListener && this.hasInputListener( this.dragListener ) && this.removeInputListener( this.dragListener ); this.dragListener && this.dragListener.dispose(); super.dispose(); } diff --git a/js/common/view/CAVPlotNode.ts b/js/common/view/CAVPlotNode.ts index e2061a40..1ce0a6f9 100644 --- a/js/common/view/CAVPlotNode.ts +++ b/js/common/view/CAVPlotNode.ts @@ -41,14 +41,12 @@ export default class CAVPlotNode extends Node { super( options ); - // TODO: Factor out height with accordion box height const backgroundNode = new Rectangle( 0, 0, CAVConstants.CHART_VIEW_WIDTH, 180 ); this.addChild( backgroundNode ); - const numberLinePositionY = 127; - // scale down in the y direction to support smaller object nodes const yScale = CAVObjectType.DATA_POINT.radius / CAVObjectType.SOCCER_BALL.radius; + const numberLinePositionY = 127; // TODO: we currently define the y range with the x width because we are thinking of it as a square, with a stack of // 15 balls as the high point. Consider instead something like above, where we just base the y scaling on the height diff --git a/js/common/view/CardNode.ts b/js/common/view/CardNode.ts index 24dcfb8b..4e81a2ab 100644 --- a/js/common/view/CardNode.ts +++ b/js/common/view/CardNode.ts @@ -84,7 +84,7 @@ export default class CardNode extends Node { start: () => { this.moveToFront(); }, - // TODO-UX: This emits for dragging the leftmost card to the left, see https://github.com/phetsims/center-and-variability/issues/150 + // TODO-UX: This emits for dragging the leftmost card to the left, see https://github.com/phetsims/center-and-variability/issues/111 drag: ( event, listener ) => this.dragDistanceEmitter.emit( Math.abs( listener.modelDelta.x ) ) } ); this.addInputListener( this.dragListener ); diff --git a/js/common/view/CardNodeContainer.ts b/js/common/view/CardNodeContainer.ts index 8dc3e949..9099a937 100644 --- a/js/common/view/CardNodeContainer.ts +++ b/js/common/view/CardNodeContainer.ts @@ -86,7 +86,7 @@ export default class CardNodeContainer extends Node { this.model = model; - // TODO-UX: maybe this should be converted to track distance for individual cards, see https://github.com/phetsims/center-and-variability/issues/150 + // TODO-UX: maybe this should be converted to track distance for individual cards, see https://github.com/phetsims/center-and-variability/issues/111 // Accumulated card drag distance, for purposes of hiding the drag indicator node const totalDragDistanceProperty = new NumberProperty( 0, { tandem: options.tandem.createTandem( 'totalDragDistanceProperty' ),