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

Missing tandems for Text/RichText elements #314

Closed
8 tasks done
pixelzoom opened this issue Jan 30, 2023 · 6 comments
Closed
8 tasks done

Missing tandems for Text/RichText elements #314

pixelzoom opened this issue Jan 30, 2023 · 6 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Jan 30, 2023

For phetsims/qa#889

Reviewing #312 resulted in me identifying some Text/RichText elements that are not instrumented, along with a couple of other misc. things.

The tandems that I believe are missing are:

Text elements:

  • concentrationScreen.view.saturatedIndicator (SaturatedIndicator.ts)
  • concentrationScreen.view.saturatedIndicator.text (SaturatedIndicator.ts)
  • simPreferences.showSolutionVolumeCheckbox.text (BLLPreferencesNode.ts)
  • simPreferences.showSoluteAmountCheckbox.text (BLLPreferencesNode.ts)
  • simPreferences.beakerUnitsControl.radioButtonGroup.litersRadioButton.text (BeakerUnitlsControl.ts)
  • simPreferences.beakerUnitsControl.radioButtonGroup.millilitersRadioButton.text (BeakerUnitlsControl.ts)
  • simPreferences.concentrationMeterUnitsControl.radioButtonGroup.molesPerLiterRadioButton.text (ConcentrationMeterUnitsControl.ts)
  • simPreferences.concentrationMeterUnitsControl.radioButtonGroup.percentRadioButton.text (ConcentrationMeterUnitsControl.ts)

I'd like to confirm with @arouinfar before I instrument these.

@pixelzoom
Copy link
Contributor Author

Confirmed with @arouinfar that the above need to be instrumented.

These do NOT need to be instrumented:

  • beersLawScreen.view.solutionPanel.solutionComboBox.listBox.*.text
  • concentrationScreen.view.solutePanel.soluteComboBox.listBox.*.text

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jan 31, 2023

These:

RichText elements:

  • concentrationScreen.view.solutePanel.soluteFormRadioButtonGroup.solidRadioButton.text (SolutionControl.ts)
  • concentrationScreen.view.solutePanel.soluteFormRadioButtonGroup.solutionRadioButton.text (SolutionControl.ts)

... turned out to be instrumented, but incorrectly named. I changed the tandem name from 'labelText' to 'text', to match the pattern used for other radio buttons.

@pixelzoom pixelzoom removed their assignment Jan 31, 2023
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Jan 31, 2023

Over to @arouinfar to review. Let me know if you want to feature any of these. Feel free to close if no further changes are needed.

@arouinfar
Copy link
Contributor

Thanks @pixelzoom. Everything looks good in master. The contents of general.model.strings.beersLawLab are featured by default. Since these new instances of text all have children linking back to the StringProperty, they'll show up in the tree when viewing featured elements. No need to feature the text itself.

@pixelzoom
Copy link
Contributor Author

Reopening to confirm that these changes made it into 1.7 release branch.

@samreid @matthew-blackman FYI.

@pixelzoom pixelzoom reopened this Feb 6, 2023
@pixelzoom
Copy link
Contributor Author

Confirmed that this change is present in 1.7. Closing.

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