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

Use NumberDisplay instead of creating unique displays #173

Closed
jbphet opened this issue Feb 25, 2019 · 7 comments
Closed

Use NumberDisplay instead of creating unique displays #173

jbphet opened this issue Feb 25, 2019 · 7 comments

Comments

@jbphet
Copy link
Contributor

jbphet commented Feb 25, 2019

There are several places in the code where a background is created and a number is placed over it, see for example createInformationBox in TracerNode. There exists a type called NumberDisplay that should do much of the work for this, and the code should be rewritten to take advantage of it. This came up while working on #169, and it would have been easier to address if NumberDisplay had been used.

@jbphet
Copy link
Contributor Author

jbphet commented Feb 25, 2019

@ariel-phet - you'd said to be on the lookout for items where @andrealin may be able to contribute. Might this be one?

@ariel-phet
Copy link

@jbphet I think this might be a nice small chunk of work for @arnabp

@ariel-phet ariel-phet assigned arnabp and unassigned ariel-phet Jun 6, 2019
@arnabp
Copy link
Contributor

arnabp commented Jun 14, 2019

When converting the parameter control boxes in the custom object of the lab screen I ran into an interesting issue. The current implementation of these text boxes simply display the number used in the Axon.NumberProperty as is.

Screen Shot 2019-06-14 at 9 02 00 AM

However, when using NumberDisplay, the option of decimal places is required. Setting that to the default of 0 removes important decimals from gravity.

Screen Shot 2019-06-14 at 9 10 19 AM

And setting it to the max (for these parameter boxes) of 2 adds unnecessary decimals to max.

Screen Shot 2019-06-14 at 9 10 45 AM

I see three ways to solve this that makes sense:

The first is to modify NumberDisplay to accommodate the issues that have come up. This involves two changes: First I'd do a null check on the decimalPlaces option that doesn't use Util.toFixed if decimalPlaces is set to null. Second would be to create a maxDecimalPlaces option that is set to decimalPlaces by default, but when overridden is used to set the width of the NumberDisplay. The first change is pretty simple, but the second adds another option which can get overcomplicated.

The second solution is to use custom decimalPlaces options for each of the parameter boxes. Unfortunately, because these boxes accept custom inputs from the user, the number of decimals in each number would need to be changed on each new user input.

The third option is to just leave these boxes and not convert them to NumberDisplay, as this problem is turning out to be more complicated than it may be worth.

Since NumberDisplay belongs to you @pixelzoom if you could take a look at this for suggestions on which way to move forward that would be great.

@pixelzoom
Copy link
Contributor

pixelzoom commented Jun 14, 2019

I understand the issue, and have some "gut" opinions. But unfortunately I don't have time to respond or consider API change to NumberDisplay, and I'll be unavailable until after July 2. If this is going to block work on Projectile Motion, then my recommendation is a variation of your third option -- leave as is for now in the sim, leave this issue open and deferred, and create a general issue for NumberDisplay in scenery-phet. Assign the scenery-phet issue to @ariel-phet, and he is welcome to assign it to me for future consideration.

@arnabp
Copy link
Contributor

arnabp commented Jul 2, 2019

I looked at the other areas where unique displays are used and, with the exception of the target label, all of them would require this change to NumberDisplay to function properly, so leaving those boxes doesn't really solve the issue as they are the basis of the issue. I've created the appropriate issue in scenery-phet to see where to go from here: phetsims/scenery-phet#511

@arnabp
Copy link
Contributor

arnabp commented Jul 11, 2019

I believe I've tackled all instances where NumberDisplay could be used, with the exception of the TracerNode, which used a String Property instead of a Number Property for its display.

@jbphet
Copy link
Contributor Author

jbphet commented Jul 12, 2019

I skimmed through the code changes and tested the behavior of the sim, all looks good. Thanks for tackling this @arnabp, 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

4 participants