Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use inheritance instead of composition for WaterLevelTriangleNode #53

Closed
pixelzoom opened this issue Jun 28, 2022 · 2 comments
Closed
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jun 28, 2022

For code review #41 ...

WaterLevelTriangleNode is effectively a slider (with a custom thumb and invisible track). But it currently extends Node, and it's name does not identify it as a slider. Using composition instead of inheritance here is not advantageous, and has disadvantages:

(1) It's a slider, and calling it something else is confusing.

(2) PhET-iO (and Studio) wants it to look like a slider. You're having to make it look like a slider in the tandem hierarchy by defining const sliderTandem, and you won't need to do that if it extends VSlider.

(3) For a11y, it should be a slider. This will become more important as other a11y features are added.

(4) If it extends VSlider, you won't need to override dispose.

So I recommend:

  • rename WaterLevelTriangleNode to WaterLevelSlider
  • reimplement so that WaterLevelTriangleNode extends VSlider
  • rename this.waterLevelTriangle to this.waterLevelSlider in WaterCup3DNode.ts, and change its tandem name to match
@pixelzoom
Copy link
Contributor Author

Similar comments about PredictMeanNode:

**
 * Representation for the draggable predict mean line
...
export default class PredictMeanNode extends AccessibleSlider( Node, 0 )

Nothing about the name of PredictMeanNode tells me that it's a line or a slider. Suggested to rename to PredictMeanLine, and document that it behaves like a slider for a11y purposes.

@marlitas
Copy link
Contributor

marlitas commented Aug 1, 2022

WaterTriangle now inherits from VSlider. Closing.

@marlitas marlitas closed this as completed Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants