phetsims / molecule-shapes

"Molecule Shapes" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/molecule-shapes
GNU General Public License v3.0
5 stars 6 forks source link

Lone Pair panel doesn't disappear when lonePairNode.visibleProperty is false #243

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 1 year ago

Test device MacBook Air M1 chip

Operating System 13.2.1

Browser Safari

Problem description For https://github.com/phetsims/qa/issues/905, when moleculeShapes.modelScreen.view.lonePairPanel.lonePairNode.visibleProperty is set to false, the Lone Pair panel remains even though it is empty. Shouldn't it disappear like the other panels do when the visible properties for everything in them are set to false?

Visuals

Screenshot 2023-02-22 at 1 33 35 PM
arouinfar commented 1 year ago

Thanks for reporting this inconsistency @Nancy-Salpepi.

I don't think it's critical for panels to disappear automatically, and sometimes there are circumstances that prevent it from happening, such as https://github.com/phetsims/circuit-construction-kit-common/issues/939. Not sure if there are extenuating circumstances here, or if it was simply an oversight.

That said, moleculeShapes.modelScreen.view.lonePairPanel.lonePairNode.visibleProperty is purposely not featured and we recommend that clients hide parents rather than individual children in the PhET-iO Guide. I am fine with closing this as wont-fix.

jonathanolson commented 1 year ago

I believe this should be fixed in master, and patched on the release branches. @Nancy-Salpepi can you verify?

Nancy-Salpepi commented 1 year ago

The lone pair panel now disappears. Thanks JO 🙂.

jonathanolson commented 1 year ago

Please close after verifying!

Nancy-Salpepi commented 1 year ago

This looks good in rc.2. Closing.

Nancy-Salpepi commented 1 year ago

Hey sorry. I have to reopen. If I set lonePairNode.visibleProperty to false, the panel disappears. If I change locale and launch the sim, it doesn't load. Here are the errors in the console:

Screenshot 2023-03-02 at 9 26 44 PM
jonathanolson commented 1 year ago

I believe I've fixed this in master and the branches. Can you verify on master?

Nancy-Salpepi commented 1 year ago

This looks fixed in master (and I verified that it doesn't happen with other panels).

zepumph commented 1 year ago

Awesome. Thanks @Nancy-Salpepi for the quick turnaround.

KatieWoe commented 1 year ago

Looks good in rc.3