Skip to content

Commit

Permalink
Address TODOs, see #45
Browse files Browse the repository at this point in the history
  • Loading branch information
samreid committed Apr 26, 2023
1 parent 196747a commit 03d318a
Show file tree
Hide file tree
Showing 6 changed files with 7 additions and 11 deletions.
2 changes: 1 addition & 1 deletion js/common/CAVConstants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'
};

Expand Down
2 changes: 0 additions & 2 deletions js/common/view/BottomRepresentationCheckboxGroup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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' } )
] ]
Expand Down
6 changes: 3 additions & 3 deletions js/common/view/CAVObjectNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
} );
Expand Down Expand Up @@ -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, {
Expand Down Expand Up @@ -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();
}
Expand Down
4 changes: 1 addition & 3 deletions js/common/view/CAVPlotNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion js/common/view/CardNode.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
Expand Down
2 changes: 1 addition & 1 deletion js/common/view/CardNodeContainer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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' ),
Expand Down

0 comments on commit 03d318a

Please sign in to comment.