search

phetsims / ohms-law

"Ohm's Law" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/ohms-law
GNU General Public License v3.0
5 stars 6 forks source link

StringTest overlap and short Ohm width #140

Closed KatieWoe closed 5 years ago

KatieWoe commented 5 years ago

Test device: Dell Operating System: Win 10 Browser: chrome Problem description: For https://github.com/phetsims/QA/issues/313 I noticed that some string tests cause the voltage/resistance strings and the variables above them can overlap. The Ohm string max width is also shorter than the corresponding V. This is likely very minor. Steps to reproduce:

  1. Go to stringTest=rtl
  2. Observe the voltage/resistance strings
  3. Go to stringTest=double or long
  4. Observe Ohm string

Screenshots: ohmsmall runstogether

Troubleshooting information (do not edit):

Name: 12345678901234567890123456789012345678901234567890 URL: https://phet-dev.colorado.edu/html/ohms-law/1.4.0-rc.1/phet/ohms-law_en_phet.html?stringTest=long Version: 1.4.0-rc.1 2019-05-01 19:41:33 UTC Features missing: touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/74.0.3729.131 Safari/537.36 Language: en-US Window: 1536x722 Pixel Ratio: 2.5/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: 16384x16384 OES_texture_float: true Dependencies JSON: {}
jessegreenberg commented 5 years ago

It looks like the maxWidth for the units takes into account the width of the value. Since "500" is wider than "4.7", the ohms units are shorter. Related commit references https://github.com/phetsims/ohms-law/issues/50.

      maxWidth: OhmsLawConstants.SLIDER_WIDTH - valueText.width - READOUT_SPACING,

The rtl issue is because the spacing of the letters symbol and label have a negative value that was chosen because it looks good with english strings.

jessegreenberg commented 5 years ago

@ariel-phet @arouinfar do you think this issue is worth looking into? We can fix the vertical spacing issue by laying out text like https://github.com/phetsims/resistance-in-a-wire/issues/106. @zepumph can you comment on whether it is important that the maxWidth of the units be dependent on the width of the value?

jessegreenberg commented 5 years ago

Eh, just doing it 🐩, The above commit handles the rtl portion of the issue.

jessegreenberg commented 5 years ago

I tested with stringTest=long, stringTest=double, stringTest=x, stringTest=rtl to make sure that #134 was still fixed after the change. Should be ready for verification in the next RC once commits are merged into 1.4 branch.

jessegreenberg commented 5 years ago

Pushed to 1.4.

KatieWoe commented 5 years ago

Looks good in 1.4.0-rc.3