phetsims / geometric-optics

Geometric Optics is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 5 forks source link

Instrument enabledProperty for virtualImageCheckbox #465

Closed Nancy-Salpepi closed 1 year ago

Nancy-Salpepi commented 1 year ago

Test device MacBook Air M1 chip

Operating System 13.3.1

Browser Safari

Problem description For https://github.com/phetsims/qa/issues/938, in studio: On both screens I noticed that all of the checkboxes have enabledProperty except for the virtualImageCheckbox. Confirmed with @arouinfar on slack that this was not intentional.

arouinfar commented 1 year ago

I think I remember now why there isn't a virtualImageCheckbox.enabledProperty in the tree. When "Light" is selected, the "Virtual Image" checkbox is disabled. The enabledProperty is read-only, so we likely decided to uninstrument it, because there isn't much value in knowing if the checkbox is disabled or not.

If that sounds correct to you @pixelzoom, we can close.

pixelzoom commented 1 year ago

Unlike the other checkboxes in this VerticalCheckboxGroup, virtualImageCheckbox has a dervied enabledProperty. When the "Light" object type is selected, this checkbox is disabled. In the above commit, I've instrumented that DerivedProperty, as shown in this diff:

          // Disable the 'Virtual Image' checkbox for lights, see https://github.com/phetsims/geometric-optics/issues/216
          enabledProperty: new DerivedProperty(
            [ opticalObjectChoiceProperty ],
            opticalObjectChoice => ( opticalObjectChoice.type !== 'light' ), {
+              tandem: options.tandem.createTandem( 'virtualImageCheckboxEnabledProperty' ),
+              phetioValueType: BooleanIO,
+              phetioFeatured: true
            } )

Unfortunately, VerticalCheckboxGroup is responsibile for creating the CheckBox instances. And there is no way to make the above virtualImageCheckboxEnabledProperty look like it is a child of the checkbox. So we currently have virtualImageCheckboxEnabledProperty as a sibiling of the checkboxes, like this:

screenshot_2512

@arouinfar please review and let me know if this is sufficient (my feeling is that it is sufficient). If it's not, then we'll have some significant common-code work to be done in VerticalCheckboxGroup.

pixelzoom commented 1 year ago

@arouinfar Also note that virtualImageCheckboxEnabledProperty is currently phetioFeatured: true. I assumed that this what you'd want, to be consistent with the other checkboxes in the group.

arouinfar commented 1 year ago

Thanks @pixelzoom. That all sounds reasonable and looks good in master, closing.