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

addAMateButton is incorrectly phetioReadOnly: true #366

Closed
samreid opened this issue Dec 16, 2024 · 2 comments
Closed

addAMateButton is incorrectly phetioReadOnly: true #366

samreid opened this issue Dec 16, 2024 · 2 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented Dec 16, 2024

As reported in phetsims/scenery#1676 (comment), the addAMateButton and subsequently its firedEmitter are read-only. However, the code comment indicates that the visibleProperty was meant to be read-only. If I understand correctly, this patch will correct the problem:

Subject: [PATCH] Use PhetioIDUtilsModule, see https://github.com/phetsims/tandem/issues/316
---
Index: js/common/view/PlayButtonGroup.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/PlayButtonGroup.ts b/js/common/view/PlayButtonGroup.ts
--- a/js/common/view/PlayButtonGroup.ts	(revision ff948a96c9fe819eb78591281b69de3729110577)
+++ b/js/common/view/PlayButtonGroup.ts	(date 1734387307472)
@@ -152,9 +152,9 @@
       cornerRadius: 5,
       xMargin: 12,
       yMargin: 8,
-      phetioReadOnly: true, // because sim state controls when this button is visible
       visiblePropertyOptions: {
-        phetioFeatured: false
+        phetioFeatured: false,
+        phetioReadOnly: true // because sim state controls when this button is visible
       },
       enabledPropertyOptions: {
         phetioFeatured: false

Please remember to regenerate the phet-io API when making this change.

@pixelzoom
Copy link
Contributor

Thanks @samreid. Done in the above commits. I also tested migration, and there were no reported problems.

@pixelzoom
Copy link
Contributor

Btw... This applied to both addAMateButton and startOverButton.

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