phetsims / gravity-and-orbits

"Gravity And Orbits" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
12 stars 6 forks source link

No max width of mass labels #391

Closed stemilymill closed 3 years ago

stemilymill commented 3 years ago

Test device Dell Operating System Win 10 Browser Chrome Problem description For phetsims/QA#657. In the To Scale screen, long strings do not scale down in the mass labels, causing the labels to become very wide.

Steps to reproduce

  1. Add the long string query parameter
  2. Select the Mass checkbox (Third down)

image

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Gravity and Orbits‬ URL: https://phet-dev.colorado.edu/html/gravity-and-orbits/1.5.0-rc.1/phet/gravity-and-orbits_all_phet.html Version: 1.5.0-rc.1 2021-06-21 17:57:44 UTC Features missing: applicationcache, generatedcontent, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/91.0.4472.106 Safari/537.36 Language: en-US Window: 1536x754 Pixel Ratio: 1.25/1 WebGL: WebGL 1.0 (OpenGL ES 2.0 Chromium) GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.0 Chromium) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 30 uniform: 4096 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 32767x32767 OES_texture_float: true Dependencies JSON: {}

arouinfar commented 3 years ago

@samreid it seems like an oversight that the mass labels don't have a maxWidth. I checked a few different locales, and I'm not seeing anything too crazy in the published version. The strings can be fairly long, even in English. For example, here's the mass label on the planet in the Planet/Satellite scene: image.

I'm not terribly convinced that the current behavior is a problem, though it does appear to against the convention of defining a maxWidth. (Maybe it does have a maxWidth and stringTest=long isn't long enough in this case?) If it turns out the mass labels don't have a maxWidth and we decide to add one, I think it'd be fine to use the same maxWidth for all mass labels and base it on the length of the string in the screenshot above. @samreid what would you recommend?

samreid commented 3 years ago

I put a maxWidth on the mass labels so that in the 2nd screen they come close but do not overlap by default:

image

@arouinfar please review in master. Mark for cherry picking and unassign if it seems good.

arouinfar commented 3 years ago

@samreid that maxWidth looks good in most cases, though the Planet's mass string in the Planet Satellite scene looks a bit too small, especially compared to other mass strings: image

If it's not too much hassle, can the Planet's mass string (in the Planet Satellite scene) have a longer maxWidth than the others?

samreid commented 3 years ago

I added the feature in the commit. @arouinfar can you please review/test in master? If it seems good, please unassign and mark as ready for cherry-picking.

arouinfar commented 3 years ago

Looks good, thanks @samreid

samreid commented 3 years ago

I cherry-picked the commits to the release branch, ready for testing when RC.2 is ready.

samreid commented 3 years ago

This can be tested by running with ?stringTest=long and checking the "mass" checkbox (3rd down) to see if the text is smaller.