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 Text and RichText #341

Closed
2 tasks done
zepumph opened this issue Aug 3, 2023 · 7 comments
Closed
2 tasks done

Uninstrument Text and RichText #341

zepumph opened this issue Aug 3, 2023 · 7 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Aug 3, 2023

In https://github.com/phetsims/phet-io/issues/1952#issuecomment-1664495275 we decided that we generally don't want to instrument Text nodes, and instead just want to focus on the stringProperties, especially if they are derived or pattern.

  • Let's double check that sim code isn't instrumenting Texts unless there is a reason (like hiding the Text).
  • Let's also look for common code UI that needs to be generally updated to support this new PhET-iO instrumented decision. (For example Natural Selection dealt with NumberDisplay and OopsDialog)
@arouinfar
Copy link
Contributor

arouinfar commented Aug 7, 2023

@jbphet please uninstrument these elements:

  • view.observationWindow.startSunlightButton.text ⚠️_Note: This is dependent upon resolving Un-instrument text in TextPushButton sun#850_
  • view.observationWindow.surfaceThermometer.comboBox.items is a container for text-related elements and can be uninstrumented
  • layerModelScreen.view.observationWindow.atmosphereLayer*.unitsStringProperty
  • layerModelScreen.view.observationWindow.surfaceThermometer.unitsStringProperty

I identified NumberDisplay and TimeControlNode as the common code elements that need to be addressed. NumberDisplay will be handled in phetsims/scenery-phet#812. I will open a separate issue for TimeControlNode. @jbphet these issues do no block dev testing of Greenhouse Effect, but should be addressed before RC.

  • layerModelScreen.view.observationWindow.atmosphereLayer*.temperatureReadout.valueText
  • layerModelScreen.view.observationWindow.surfaceThermometer.temperatureReadout.valueText
  • timeControlNode.speedRadioButtonGroup.*RadioButton.labelText

@arouinfar
Copy link
Contributor

NumberDisplay will be handled in phetsims/scenery-phet#812

Actually, @jbphet I think we can uninstrument the NumberDisplays used on the Layer Model screen instead.

@arouinfar arouinfar assigned jbphet and unassigned arouinfar Aug 7, 2023
jbphet added a commit that referenced this issue Aug 7, 2023
@jbphet
Copy link
Contributor

jbphet commented Aug 7, 2023

I've done some of these, but have questions on some. I'll request some time with @arouinfar, and I think we'll be able to finalize this pretty quickly.

Worthy of note: I'm not doing anything in the GE code to instrument view.observationWindow.startSunlightButton.text, it's happening in TextPushButton.ts.

@jbphet
Copy link
Contributor

jbphet commented Aug 8, 2023

Before these requests can be fully addressed, we need to have resolutions to the following common code issue, and they need to be included the RC branch:

@arouinfar
Copy link
Contributor

Thanks @jbphet the changes look good in master. I also added phetsims/joist#934 to the list in #341 (comment).

@jbphet
Copy link
Contributor

jbphet commented Aug 18, 2023

@arouinfar - Can you check whether we've addressed all of these? I've looked it over some and I think we have gotten rid of most if not all of the instrumented text, but you know more about it than I.

@jbphet jbphet assigned arouinfar and unassigned jbphet Aug 18, 2023
@arouinfar
Copy link
Contributor

@jbphet I reviewed on master, and things are looking great. I couldn't find any extraneous instrumentation of Text/RichText, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants