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 initialConcentrationControl.spinner.numberDisplay or address common-code issue #197

Closed
pixelzoom opened this issue Aug 1, 2023 · 4 comments

Comments

@pixelzoom
Copy link
Contributor

Related to phetsims/scenery-phet#812 ...

There is 1 NumberDisplay in acid-base-solutions:

  • acidBaseSolutions.mySolutionScreen.view.solutionPanel.initialConcentrationControl.spinner.numberDisplay

It's valueText is instrumented, and we don't want it to be. It's stringProperty is buggy, missing a dependency. See phetsims/scenery-phet#812.

Options:
(1) Do nothing, leave it instrumented. This will result in an API change and migration rule in the future, so not recommended.
(2) Uninstrument this element. This is the easiest option. initialConcentrationControl.spinner.property links to the model value, but will show more decimal places than are actually displayed.
(3) Address phetsims/scenery-phet#812.

@pixelzoom pixelzoom changed the title Uninstrument initialConcentrationControl.spinner.numberDisplay or address common-code issue? Uninstrument initialConcentrationControl.spinner.numberDisplay or address common-code issue Aug 2, 2023
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Aug 3, 2023

The consensus on a lighterweight plan for addressing NumberDisplay is documented in phetsims/scenery-phet#812 (comment). We can consider this when we come to it in this sim, if no other sim has addressed it yet.

In the case of this sim, I'm leaning towards uninstrumenting spinner.numberDisplay -- option (2) in #197 (comment). spinner.property seems adequate here, despite the fact that it will have more decimal places. And we can revisit if someone asks in the future (which is doubtful).

@arouinfar arouinfar self-assigned this Aug 9, 2023
@arouinfar
Copy link
Contributor

I agree, @pixelzoom. Let's uninstrument spinner.numberDisplay.

When it comes to NumberSpinner, I don't see any reason why NumberDisplay needs to be instrumented at all. Is this something that we can handle in sun?

@arouinfar arouinfar assigned pixelzoom and unassigned arouinfar Aug 10, 2023
@pixelzoom
Copy link
Contributor Author

Let's uninstrument spinner.numberDisplay

Done. Back to @arouinfar for review. Close if OK.

@arouinfar
Copy link
Contributor

Looks good, thanks @pixelzoom!

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