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

Virtual Lab: Hiding ammeter icon in toolbox should also hide the 'Ammeter' label #987

Closed arouinfar closed 1 year ago

arouinfar commented 1 year ago

Discovered in https://github.com/phetsims/circuit-construction-kit-dc-virtual-lab/issues/20

In cck-dc, the 'Ammeters' label disappears when BOTH types of ammeter icons have been hidden. In cck-dc-virtual-lab, there is only one kind of ammeter icon: view.sensorToolbox.seriesAmmeterToolNode. When its visibleProperty is false, the 'Ammeter' label in the toolbox should automatically hide. Instead, it looks like this:

image

samreid commented 1 year ago

I added a fix that addresses the visibility for the ammeter. It seems to work well in DC-VL and DC. I noticed the voltmeter doesn't center when the ammeters disappear, but that seems also present in DC so is presumably not in scope for this publication. @arouinfar can you please test in master?

NOTE: If this solution is OK, then after creating SHAs as described in https://github.com/phetsims/circuit-construction-kit-dc-virtual-lab/issues/20 we would probably cherry-pick this into those branches.

matthew-blackman commented 1 year ago

This looks like a resurgence of https://github.com/phetsims/circuit-construction-kit-common/issues/925

@arouinfar would like your input as to whether this warrants 30-60 mins of work and a maintenance release on CCK-DC.

zepumph commented 1 year ago

We were able to reproduce this in the published CCK DC studio by hiding circuitConstructionKitDc.introScreen.view.sensorToolbox.noncontactAmmeterToolNode.visibleProperty.

Let's do a quick MR for this!

KatieWoe commented 1 year ago

Looks mostly good. I did notice that, when the ammeter is removed the voltmeter shifts but doesn't center. However, when the voltmeter is removed the ammeter does center. Is this acceptable? vgone gone notgone

samreid commented 1 year ago

We observed the same problem in https://github.com/phetsims/circuit-construction-kit-dc-virtual-lab/issues/20#issuecomment-1464200104 and concluded that it was not important enough to delay publication.

KatieWoe commented 1 year ago

In that case this is probably done

arouinfar commented 1 year ago

Thanks, everyone. I agree this is not a show-stopper for publication.

From @zepumph's https://github.com/phetsims/circuit-construction-kit-common/issues/987#issuecomment-1464239422, and the referenced commits/issue after, it looks like we are planning to proceed with a maintenance release later. Is that correct? If so, that sounds good to me.

Since @KatieWoe has verified for the purposes of https://github.com/phetsims/qa/issues/919, I'll send this back to devs for the future MR.

arouinfar commented 1 year ago

Let's continue the discussion of the layout issue in https://github.com/phetsims/circuit-construction-kit-common/issues/925

@KatieWoe confirmed the bug reported here has been addressed, closing.

@samreid confirmed that the fixes have already made their way over to cck-dc, so nothing to do here.

zepumph commented 1 year ago

MR for CCK DC 1.3 has been completed.