Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Nancy-Salpepi opened this issue Feb 15, 2022 · 8 comments
Closed

Comments

@Nancy-Salpepi
Copy link

Test device
Dell

Operating System
Win10

Browser
Chrome

Problem description
For phetsims/qa#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

visibleproperty.mp4
@Nancy-Salpepi
Copy link
Author

Nancy-Salpepi commented Feb 15, 2022

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.

metersvisible.mp4

@samreid
Copy link
Member

samreid commented Jan 5, 2023

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:

  • ammeterToolNode and voltmeterNodeIcon have inconsistent names.
  • Hiding either of those leaves behind its title:

image

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

image

@samreid
Copy link
Member

samreid commented Jan 5, 2023

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

@samreid
Copy link
Member

samreid commented Jan 5, 2023

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
Copy link
Member

samreid commented Jan 5, 2023

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
Copy link
Contributor

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

@arouinfar
Copy link
Contributor

arouinfar commented Jan 6, 2023

@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?

@arouinfar arouinfar assigned samreid and unassigned arouinfar Jan 6, 2023
samreid added a commit to phetsims/circuit-construction-kit-dc that referenced this issue Jan 7, 2023
samreid added a commit that referenced this issue Jan 7, 2023
@samreid
Copy link
Member

samreid commented Jan 7, 2023

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

@samreid samreid closed this as completed Jan 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants