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

Uninstrument opticSurfaceTypeRadioButtonGroup for the Mirror screen in Basics #463

Closed
pixelzoom opened this issue May 4, 2023 · 4 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

Related to #459 ...

geometric-optics-basics-overrides.js has been mostly migrated into code, and this is what remains:

window.phet.preloads.phetio.phetioElementsOverrides =
  {
    "geometricOpticsBasics.mirrorScreen.view.controls.opticSurfaceTypeRadioButtonGroup.enabledProperty": {
      "phetioFeatured": false
    },
    "geometricOpticsBasics.mirrorScreen.view.controls.opticSurfaceTypeRadioButtonGroup.flatRadioButton.enabledProperty": {
      "phetioFeatured": false
    },
    "geometricOpticsBasics.mirrorScreen.view.controls.opticSurfaceTypeRadioButtonGroup.flatRadioButton.visibleProperty": {
      "phetioFeatured": false
    },
    "geometricOpticsBasics.mirrorScreen.view.controls.opticSurfaceTypeRadioButtonGroup.visibleProperty": {
      "phetioFeatured": false
    }
  };

The basics version has only 1 radio button, the "flat" optic shape. And the goal of the above is to unfeature the "group" and the flat button. But migrating the above into code is going to be a mess. And I don't see any value in making this button visible -- it would in fact be very weird. So I propose that we totally uninstrument geometricOpticsBasics.mirrorScreen.view.controls.opticSurfaceTypeRadioButtonGroup -- which will be very easy.

@arouinfar thoughts?

@pixelzoom
Copy link
Contributor Author

Here's the patch to uninstrument opticSurfaceTypeRadioButtonGroup for the Mirror screen in Basics. This would be followed by removing the remaining entries from geometric-optics-basics-overrides.js, then regenerating API files. There should be a change to geometric-optics-basics-phet-io-api.json, and no change to geometric-optics-phet-io-api.json.

patch
Subject: [PATCH] partial migration of overrides.js into code, https://github.com/phetsims/geometric-optics/issues/459
---
Index: js/common/view/GOScreenView.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/GOScreenView.ts b/js/common/view/GOScreenView.ts
--- a/js/common/view/GOScreenView.ts	(revision 3fb2d0a0d7073ebb4fdcd9cff7464fa0a0dda8f6)
+++ b/js/common/view/GOScreenView.ts	(date 1683171151303)
@@ -47,6 +47,8 @@
 import GOToolNode from './tools/GOToolNode.js';
 import StringUnionProperty from '../../../../axon/js/StringUnionProperty.js';
 import Multilink from '../../../../axon/js/Multilink.js';
+import Mirror from '../../mirror/model/Mirror.js';
+import Tandem from '../../../../tandem/js/Tandem.js';
 
 // Zoom scale factors, in ascending order.
 // Careful! If you add values here, you may get undesirable tick intervals on rulers.
@@ -247,7 +249,12 @@
     const opticSurfaceTypeRadioButtonGroup = new OpticSurfaceTypeRadioButtonGroup( model.optic, {
       centerX: erodedLayoutBounds.centerX,
       top: erodedLayoutBounds.top,
-      tandem: controlsTandem.createTandem( 'opticSurfaceTypeRadioButtonGroup' )
+
+      // Do not instrument for the Mirror screen in Geometric Optics: Basics
+      // See https://github.com/phetsims/geometric-optics/issues/463
+      tandem: ( options.isBasicsVersion && model.optic instanceof Mirror ) ?
+              Tandem.OPT_OUT :
+              controlsTandem.createTandem( 'opticSurfaceTypeRadioButtonGroup' )
     } );
     opticSurfaceTypeRadioButtonGroup.visible = !options.isBasicsVersion;
 

@pixelzoom pixelzoom changed the title Uninstrument geometricOpticsBasics.mirrorScreen.view.controls.opticSurfaceTypeRadioButtonGroup Uninstrument opticSurfaceTypeRadioButtonGroup for the Mirror screen in Basics May 4, 2023
@pixelzoom
Copy link
Contributor Author

If we do this, reminder to myself to make this change in geometric-optics-basics/doc/implementation-notes.md:

- Since there is only one mirror shape, hide the radio button. It is possible to make it visible via PhET-iO.
+ Since there is only one mirror shape, hide the radio button. It is not possible to make it visible via PhET-iO, because it is not instrumented.

@arouinfar
Copy link
Contributor

And I don't see any value in making this button visible -- it would in fact be very weird. So I propose that we totally uninstrument geometricOpticsBasics.mirrorScreen.view.controls.opticSurfaceTypeRadioButtonGroup -- which will be very easy.

I completely agree @pixelzoom. Let's uninstrument it.

@arouinfar arouinfar assigned pixelzoom and unassigned arouinfar May 4, 2023
pixelzoom added a commit to phetsims/geometric-optics-basics that referenced this issue May 5, 2023
@pixelzoom
Copy link
Contributor Author

Done in the above commits. There's really nothing to review here -- this simply removed geometricOpticsBasics.mirrorScreen.view.controls.opticSurfaceTypeRadioButtonGroup, and all other occurrences of opticSurfaceTypeRadioButtonGroup are unchanged. So I won't have @arouinfar review, and will simply close this issue.

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

2 participants