phetsims / scenery

Scenery is an HTML5 scene graph.
MIT License
54 stars 12 forks source link

Make Node phetioVisiblePropertyInstrumented: false #1533

Open samreid opened 1 year ago

samreid commented 1 year ago
Subject: [PATCH] Allow DisplayOptionsPanel to collapse when all contents are hidden, see https://github.com/phetsims/circuit-construction-kit-common/issues/939
---
Index: js/nodes/Node.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/nodes/Node.ts b/js/nodes/Node.ts
--- a/js/nodes/Node.ts  (revision 73d43c70bb11af424250a553e3cc27b31064f203)
+++ b/js/nodes/Node.ts  (date 1675361824300)
@@ -261,7 +261,7 @@
 ];

 const DEFAULT_OPTIONS = {
-  phetioVisiblePropertyInstrumented: true,
+  phetioVisiblePropertyInstrumented: false,
   visible: true,
   opacity: 1,
   disabledOpacity: 1,

From a meeting today with @pixelzoom @zepumph @arouinfar @matthew-blackman @pixelzoom. We would like to prune more visibleProperties by making the change above. Sun components should opt in. Sim-specific components should opt in.

Also discussed alongside https://github.com/phetsims/sun/issues/826

zepumph commented 1 year ago

I would treat this more as an experiement than an implementation. The goal would be to understand the ramifications of this swap. How many places do we NEED to add a sim specific opt in. If it is more than the opt-out benefits, then perhaps we would want to keep this as is. I'm not too sure.

pixelzoom commented 1 year ago

While I love the idea of opting in to visibleProperty, I'm afraid that this is now going to be impossible. We have no idea what visibleProperty elements are being used by clients, and no way to address this in an acceptable manner via a migration rule.

samreid commented 1 year ago

We can safely make this change in master using the API files as a safety net. Once we change it in Node and Sun, we can regenerate API files and see how much work is there.

kathy-phet commented 1 year ago

Not sure if this should be here or in the other discussion ... so pasting here too. @pixelzoom @arouinfar @zepumph @matthew-blackman , @samreid - Just a question about this. If I'm understanding, you won't be able to hide or disable one of the radio buttons when there are 2. One question here ... sometimes these radiobuttons serve as a bit of a legend so you want to keep it visible, but you want the student to stay on one of the 2 specific radio buttons for a while. It sounds like uninstrumenting will mean the instructional designer won't be able to do this? Keep the 2 radio buttons, but disable the one they don't want students to select (or they don't want students to select at the moment).

If the above is accurate description, I would vote for just leaving it instrumented and keep the flexibility here.

samreid commented 1 year ago

Tagging for https://github.com/phetsims/phet-io/issues/1914

marlitas commented 1 year ago

While triaging issues for POSE I noticed that the majority vote here seems to be to leave as-is. I also noticed that this issues was connected to https://github.com/phetsims/phet-io/issues/1914 which is now closed. @samreid can we close this issue?

zepumph commented 1 year ago

Though this issue is old, it deserves conversation.

I noticed that the majority vote here seems to be to leave as-is.

Where are you seeing this? I think this could be one of the most important changes to prune down useless PhET-iO elements and simplify the API.

zepumph commented 1 year ago

I also noticed that this issues was connected to https://github.com/phetsims/phet-io/issues/1914 which is now closed

Looks like we are using a project board for the backlog now.

marlitas commented 1 year ago

Where are you seeing this?

I just read through the CM, KP, and MK comments and it sounded like all three were generally recommending to leave as-is, but perhaps I misunderstood. We can definitely keep this open if that's the case, just was flagging what appeared to be an issue that could be closed, but happy to be wrong!