phetsims / beers-law-lab

"Beer's Law Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/beers-law-lab
GNU General Public License v3.0
2 stars 9 forks source link

Uninstrument cuvetteNode.visibleProperty #328

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 1 year ago

Test device MacBook Air (m1 chip)

Operating System 13.1

Browser chrome 110

Problem description

Discussed with @arouinfar on Slack

For https://github.com/phetsims/qa/issues/894, on the Beer's Law Screen it is possible to set the cuvetteNode.visibleProperty to false. There is no reason to hide the cuvette (and the data from the detector still reads as though the cuvette is present). Is it possible to uninstrument (or if not, to un-feature)?

This will also need to be deleted from the overrides file.

Visuals

Screenshot 2023-02-08 at 4 14 15 PM
pixelzoom commented 1 year ago

Yes, it's possible to uninstrument or unfeature beersLawLab.beersLawScreen.view.cuvetteNode.visibleProperty. If there's no reason to hide it, I recommend uninstrumenting. @arouinfar is that OK with you?

pixelzoom commented 1 year ago

While we're at it... Is there any reason to hide the AT Detector? Should we also uninstrument beersLawLab.beersLawScreen.view.detectorNode.visibleProperty?

arouinfar commented 1 year ago

If there's no reason to hide it, I recommend uninstrumenting. @arouinfar is that OK with you?

Yes, uninstrumenting would be my preference.

While we're at it... Is there any reason to hide the AT Detector?

Hiding the detector doesn't lead to broken-looking situations like the cuvette or light #310, so we can probably leave it alone. It could be useful to temporarily hide for prediction purposes, but there are other alternatives (like moving probe out of the path). I don't have a strong feeling either way, so I'll let you make that call @pixelzoom.

pixelzoom commented 1 year ago

OK, I guess we'll keep beersLawLab.beersLawScreen.view.detectorNode.visibleProperty for its hypothetical usefulness.

beersLawLab.beersLawScreen.view.cuvetteNode.visibleProperty was uninstrumented in the above commits to master. So it no longer exists. See screenshot below.

I still need to cherry-pick to 1.7. That involves beers-law-lab and phet-io-sim-specific.

screenshot_2296
pixelzoom commented 1 year ago

Cherry-picks completed, ready for verification in next RC.

pixelzoom commented 1 year ago

Verification in the next RC should include the following for beers-law-lab only:

stemilymill commented 1 year ago

looks good