search

phetsims / graphing-quadratics

"Graphing Quadratics" is an educational simulation in HTML5, by PhET Interactive Simulations.
MIT License
1 stars 4 forks source link

Exposing RichText when instrumenting labels and titles #63

Closed chrisklus closed 6 years ago

chrisklus commented 6 years ago

StandardFormEquationNode extends RichText so @zepumph and I think that it should be renamed to StandardFormEquationText.

Found while working on #14.

pixelzoom commented 6 years ago

There is no need to expose the implementation detail that it's a subtype of RichText (or a single any kind of text). In fact I wanted to explicitly discourage that because other similar equations (e.g. FocusAndDirectrixEquationNode) cannot be implemented with RichText, and are an assemblage of many Nodes.

Does that make sense?

chrisklus commented 6 years ago

Does that make sense?

It does, thanks for clarifying. Closing.

pixelzoom commented 6 years ago

Reopening because this does bring up a PhET-iO question. StandardFormEquationNode is instrumented. And since it extends RichText, that gives the client access to the full PhET-iO API for RichText - which we don't want. So perhaps I should make StandardFormEquationNode extend Node, and hide the RichText as a child. What do you think?

pixelzoom commented 6 years ago

I went ahead and wrapped the RichText, see above commit. I think this is appropriate because the RichText API is not supposed to be visible to clients (PhET-iO or otherwise). And now that StandardFormEquationNode extends Node, hopefully the class name is more agreeable. If this looks OK to you, feel free to close.

chrisklus commented 6 years ago

@zepumph and I think that sounds like a great idea, nice change. Closing.

zepumph commented 6 years ago

That commit is better than a solution where we pass in phetioType: NodeIO to cut out the RichText functionality from the type.

chrisklus commented 6 years ago

@zepumph and I are also wondering if the labels for the coefficient sliders (a, b, c) should be exposing RichText API as they currently are.

pixelzoom commented 6 years ago

I think the slider labels are fine as RichText. For the general programming API, they aren't visible outside of CoefficientSlider. And for PhET-iO we decided that "all or nothing" was OK, so I'm not going to go around wrapping everything in a Node to protect it against nefarious PhET-iO usage.

zepumph commented 6 years ago

This question extends to how we are treating Checkboxes in GQ too. Currently the text isn't instrumented. In FL the checkbox labels aren't instrumented (see voltmeterCheckbox in ControlPanelNode.js), but this seems inconsistent with having these labels instrumented. @pixelzoom what do you think is best practice (as a whole) for graphing quadratics? In my mind the options are:

  1. Instrument all labels/titles/text that accompanies UI
  2. Uninstrument these instances and just have the encapsulating Node instrumented to toggle visibility and such.
pixelzoom commented 6 years ago

The PhET-iO design requirements for this sim did not include any ability to change text, on Checkboxes or any other UI components. The requirements related to things that have text were the ability to show/hide:

"Instrument all labels/titles/text that accompanies UI" sounds like a step back toward the "instrument everything" approach, and I don't think that is desirable or necessary.

zepumph commented 6 years ago

Sounds good. I feel like we are ready to close this issue, as it sounds like the current approach meets the phet-io design requirements better than any proposed general convention does.