search

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

Sensor tool icons should reflow centered layout when others are made invisible #925

Closed matthew-blackman closed 1 year ago

matthew-blackman commented 1 year ago

If a sensor tool icon is removed via PhET-iO, the rest of the sensors should reflow so that they are center-aligned. This applied both within the sensor toolbox as well as the subset of ammeter types.

This issue was discovered in https://github.com/phetsims/circuit-construction-kit-common/issues/917

matthew-blackman commented 1 year ago

Things are looking good in the sensor toolbox tree and ready for @arouinfar to review. Note that @samreid and I are aware of a panel alignment issue (see screenshot below) that is exposed by removing tool nodes, but is unrelated to this issue. I will open a separate issue for this.

Screen Shot 2023-01-19 at 2 31 17 PM
arouinfar commented 1 year ago

@matthew-blackman looks good! Seems like the AccordionBox alignment hasn't been moved to its own issue yet, so I'll leave this one open so we don't forget.

matthew-blackman commented 1 year ago

Moved remaining panel layout issues to https://github.com/phetsims/circuit-construction-kit-common/issues/940

matthew-blackman commented 1 year ago

While working on the featured overrides, @arouinfar and I noticed that the nonContactAmmeterToolNode does not have a visible property. It should have the same tree structure as the voltmeterToolNodes and seriesAmmeterToolNodes

samreid commented 1 year ago

I added that, and it seems to work well in testing. Closing.

arouinfar commented 1 year ago

Looks like there's a regression and this is no longer fixed, as discovered in https://github.com/phetsims/circuit-construction-kit-common/issues/987.

matthew-blackman commented 1 year ago

The above commit appears to fix the issue for both DC and DC-VL. @arouinfar please confirm that all combos of voltmeter/ammeters visibility reflow correctly. @samreid if you can code review https://github.com/phetsims/circuit-construction-kit-common/commit/2ff682bcb5d25c5a5895a1a66d0c382bbb38d44a I think we should be able to MR this into CCK common.

matthew-blackman commented 1 year ago

Commit https://github.com/phetsims/circuit-construction-kit-common/commit/2ff682bcb5d25c5a5895a1a66d0c382bbb38d44a causes a resurgence of https://github.com/phetsims/circuit-construction-kit-common/issues/925.

@samreid @arouinfar and I agree that https://github.com/phetsims/circuit-construction-kit-common/issues/925 should be a priority here, since it can appear during student interaction with sim. At this time, we are not going to put more effort into this layout issue. Closing.