-
Notifications
You must be signed in to change notification settings - Fork 6
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
is ZoomButton over-instrumented? #573
Comments
I agree, it seems like a symptom of the instrument everything approach, and those extraneous instrumentations should be removed. |
@arouinfar or @kathy-phet if you agree, I'll proceed with removing instrumentation of the icon. |
Not only is this over-instrumented, but the instrumentation is buggy, and was probably not tested. If I set |
It's definitely over instrumented @pixelzoom. I see no reason why the icon would be instrumented, rather than the button as a whole. |
Thanks @arouinfar. I'll proceed which changes. |
@arouinfar could you please verify in ph-scale 1.4.0-dev.5 ? In Studio, navigate to |
Looks wonderful @pixelzoom |
Related to https://github.com/phetsims/ph-scale/issues (ph-scale instrumentation).
ZoomButton
is a common-code UI component. Here's what it looks like in ph-scale. There are 2 of them below the linear graph in the Macro screen:Here's the Studio tree structure for one of those ZoomButtons:
This looks over-instrumented to me, perhaps vestigial from the "instrument everything" days of PhET-iO. Why are we instrumenting the parts of the icon (
glassNode
,signNode
) separately? If instrumenting the icon is important, why isn't the icon (as a whole) instrumented?I recommend that we remove instrumentation of the icon, and make the instrumentation similar to ResetAllButton and other push buttons, i.e.:
(EDIT: Note that
isFiringProperty
is specific to ResetAllButton. According to the doc, it's "Commonly used to disable audio effects during reset".)Assigning to PhET-iO designers to review. If someone from the PhET-iO dev team knows why the icon is instrumented this way, please chime in.
The text was updated successfully, but these errors were encountered: