phetsims / center-and-variability

"Center and Variability" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 2 forks source link

Misc PhET-iO instrumentation #496

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

For code review #447 ...

Studio tree looks nice.

For UI components, we typically use tandem names that match the UI labels, so that they are easier to discover in Studio, and the Studio tree is a bit easier to grok when browsing. These element names violate that convention:

Misc things:

samreid commented 1 year ago

Should the Preferences > Simulation controls be instrumented?

We were working based on this remark, is it incorrect or out of date? https://github.com/phetsims/center-and-variability/blob/4a73eccd9971f5d70788e3fef4523d183a022260/js/common/view/SimulationPreferencesContentNode.ts#L45-L46

catherinecarter commented 1 year ago

For the accordion box titles mentioned above:

plotAccordionBox => linePlotAccordionBox variabilityMeasureAccordionBox => rangeAccordionBox

Each of these accordion boxes can change its name. For the plotAccordionBox, in preferences, if a dotplot is chosen as the display, this accordion box title changes to dotPlotAccordionBox. To be generic, this accordion box was named plotAccordionBox. I agree this could have a better name, but this seemed like a good compromise.

Same for the variabilityMeasureAccordionBox. If the user changes the radio button in the accordion box, the title changes to match the measure of variability. For the range, the title is Range. For the IQR, the title is "Interquartile Range (IQR)," and the for the MAD, the title is, "Mean Absolute Deviation." To capture them all, we named it variabilityMeasureAccordionBox. Wish we could have the key in the studio be dynamic to match the title of the Accordion Box.

Nice catch on the others!

samreid commented 1 year ago

I renamed the other tandems. Returning the issue to @pixelzoom to spot check and comment on https://github.com/phetsims/center-and-variability/issues/496#issuecomment-1688629348

pixelzoom commented 1 year ago

Commits look good.

Re https://github.com/phetsims/center-and-variability/issues/496#issuecomment-1688629348, I don't believe that code comment is correct. At a minimum, we want instructional designers to be able to hide individual sim-specific Preferences controls. And that's what we've done in 100% of the sims that I've instrumented that have Preferences: acid-base-solutions, beers-law-lab, calculus-grapher, gas-properties, geometric-optics, molecule-polarity. You can also consult/confirm with @arouinfar.

samreid commented 1 year ago

Thanks, I agree that is the way to go. I implemented it in the commits. @catherinecarter can you please review the tandems and tree? It looks like this for me:

image

catherinecarter commented 1 year ago

I looked at Beers Law Lab for an example, and it looks like we need to add a linked property under each of the preferences, pointing users to the global.model.preferences where the actual values of the preferences can be changed. Currently it looks like the +plotTypeRadioButtonGroup is the only one featured, and the only one with a linked property.

samreid commented 1 year ago

I instrumented the controls within each "row". I uninstrumented the the control within the row's visible property, so you can just hide or show the entire row. The tree now looks like this:

image

It was unclear from the comment if these should be featured, but it sounds helpful to feature them (and their visible properties), so I did that too. Ready for @catherinecarter to review by testing in studio.

catherinecarter commented 1 year ago

I tested this by only showing the featured elements, and I could see all the preferences and their linked properties. In my somewhat limited knowledge of what should/shouldn't be featured, it looks good to me. Thanks for attending to this.

samreid commented 1 year ago

Thanks, closing.