phetsims / number-line-integers

"Number Line: Integers" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 4 forks source link

Temperature units can overlap with number line #63

Closed chrisklus closed 4 years ago

chrisklus commented 4 years ago

For https://github.com/phetsims/number-line-integers/issues/62.

I'm cheating the test a bit in order to get these results, but I noticed that with stringTest=double, the temperature units radio button was touching the number line, so at lengths like 4x, the buttons/labels overlap with the number line. This isn't caught by stringTest=long because their height gets small enough to be out of the way of the number line.

image

SaurabhTotey commented 4 years ago

A similar issue appears when the title text is shorter. I pushed a possible fix that positions the number line relative to that heading section rather than just the title. However, this may not look as nice in a non-stringtest version with a long enough title because then there is some unused space above the number line.

jbphet commented 4 years ago

I decided to tackle this a little differently than what @SaurabhTotey proposed. It seems to me that the number line should stay consistently positioned roughly in the center of the panel, but the labels on the left side were causing the number lines to move when the label lengths changed. I backed out the code that dynamically repositioned the number line and modified the layout code to have less interdependence on the size and position of other portions of the panel, and made the number line position itself based off the right side where there aren't any labels. My goal was to make it so that the number line position stayed constant regardless of the label sizes, but I didn't quite get there, and felt like the behavior that I did get was reasonable (and I thought I should move on to other issues).

@chrisklus - as the code reviewer, please take a look at the revised behavior and the code changes and let me know if this seems like a reasonable solution (and feel free to close if so).

chrisklus commented 4 years ago

I'm still seeing a collision when stringTest is at 3 or 4 x:

image

Since the temperature units are to the right of the number line, the number line's right can't be hard coded unless the number line is moved down like @SaurabhTotey proposed. @jbphet if you want it to stay in the same place in relation to the right side of the panel but don't want to use a dynamic layout (in the vertical direction), perhaps you can shift the number line to always be below the temperature units.

jbphet commented 4 years ago

@chrisklus said:

I'm cheating the test a bit in order to get these results[.]

What exactly are you doing? I'll continue to try to refine it, but I'd like to know what test you're running so that I can easily replicate the results.

jbphet commented 4 years ago

I replicated the results by hacking the window.phet.chipper.mapString function definition in initialize-globals.js, but I'm still interested in know what test is actually being run here since I'm not familiar with 3x and 4x string tests and I'm not seeing how to run them by examining the code in initialize-globals.

@chrisklus said:

[P]erhaps you can shift the number line to always be below the temperature units.

I'd rather not do this, since it doesn't match the design doc and and don't think it would look as good. Also, this seems like a corner case, and I don't want to compromise the more nominal cases to handle it.

I've added an empirically determined maxWidth option for the units strings, and that seems to handle my "hack" test to get 4x string. Here's a screenshot:

image

I tried this for stringTest=long, double, rtl and 3x version of the "hack test". All looked reasonable to me. Back to @chrisklus for more discussion or closure.

chrisklus commented 4 years ago

I replicated the results by hacking the window.phet.chipper.mapString function definition in initialize-globals.js, but I'm still interested in know what test is actually being run here since I'm not familiar with 3x and 4x string tests and I'm not seeing how to run them by examining the code in initialize-globals.

That's what I did as well, sorry for not clarifying. I only tested these lengths in addition to stringTest=double since I originally noticed that the true stringTest=double was very close to causing the unit strings to overlap with the number line.

Nice fix, all tests look good to me as well. Closing.