phetsims / circuit-construction-kit-common

"Circuit Construction Kit: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
10 stars 10 forks source link

Is the circuit element tool visible property working correctly? #858

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 2 years ago

Test device Dell

Operating System Win10

Browser Chrome

Problem description For https://github.com/phetsims/qa/issues/774:

In Studio, when a circuit element that there is multiple of, such as a battery, is placed in the play area and then its visible property is set to false (view.circuitElementToolbox.circuitElementTools.rightBatteryToolNode.visibleProperty), it's value will be true when the sim is launched. If that item was not in the play area, its value stays false.

This seems like a bug to me, but I first wanted to make sure that this was not by design.

Steps to reproduce

  1. In either screen, place a circuit element there is multiple of (battery, switch, wire, resistor, fuse, bulb) into the play area
  2. Set its visible property to false--the circuit element disappears from the carousel
  3. Launch the sim--the circuit element appears in the carousel and more of that element can be moved to the play area

Visuals

https://user-images.githubusercontent.com/87318828/154090172-5b14ec67-e89a-460f-97e2-bf3c295a6edc.mp4

Nancy-Salpepi commented 2 years ago

I noticed that if I follow the same steps as above, but with the ammeter (view.sensorToolbox.ammeterToolNode.visibleProperty) and voltmeter (view.sensorToolbox.voltmeterNodeIcon.visibleProperty) the visibleProperty stays false after launching.

When I return the meters to the panel, they disappear.

https://user-images.githubusercontent.com/87318828/154117847-8ec8399e-693a-466a-b636-7764493ef59d.mp4

samreid commented 1 year ago

I tested all the functionality described above and it now seems to be working properly (transmitted to preview sim correctly). But I observed the following issues:

image

Those labels cannot be hidden but they can get their strings set to the empty string:

image
samreid commented 1 year ago

I improved the tandem name for the voltmeter so it is consistent with the rest of the sim.

samreid commented 1 year ago

The labels were supposed to auto-hide when you hide a voltmeter or ammeter tool node, I corrected that in the commit. I also added a TODO since I was surprised to see 2 levels in that hierarchy (last 2 in this list):

image
samreid commented 1 year ago

Anyways, I don't think I should work on that tree much more until checking in about the design of that part. Do we still need ammeterToolIconWithLabel? Should seriesAmmeterNodeIcon be nested under seriesAmmeterToolNode? Should the labels be top-level in sensorToolbox, and auto-hide?

arouinfar commented 1 year ago

I'll review this with @matthew-blackman in #917

arouinfar commented 1 year ago

@matthew-blackman and I verified that the labels in the sensor toolbox disappear when hiding the corresponding ToolNode. The "Ammeters" text will disappear if both seriesAmmeterToolNode and ammeterToolNode are hidden. This all looks good.

@samreid There isn't a reason to keep the *IconWithLabel. Can you remove those?

samreid commented 1 year ago

I removed the iconWithLabel and regenerated APIs. Tested in Studio. Closing.