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

Strings may not be vertically centered #448

Closed KatieWoe closed 1 year ago

KatieWoe commented 1 year ago

Test device Dell Operating System Win 11 Browser Chrome Problem description For https://github.com/phetsims/qa/issues/838. Text may not be vertically centered. This is especially the case when strings are longer than in English. This can be done with ?stringTest=long or with existing translations. This can be easily seen with the icons in the nav bar. This seems to be related to https://github.com/phetsims/joist/issues/645.

Visuals Current RC:

off

Published Sim:

onn

Troubleshooting information:

!!!!! DO NOT EDIT !!!!! Name: ‪Gravity and Orbits‬ URL: https://phet-dev.colorado.edu/html/gravity-and-orbits/1.6.0-rc.1/phet/gravity-and-orbits_all_phet.html Version: 1.6.0-rc.1 2022-09-27 17:23:51 UTC Features missing: applicationcache, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/106.0.0.0 Safari/537.36 Language: en-US Window: 1280x649 Pixel Ratio: 1.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: 32767x32767 OES_texture_float: true Dependencies JSON: {}
samreid commented 1 year ago

Collaborating with @zepumph and @jonathanolson, we also saw that GridBox wasn't yAlign: 'top' as we expected, when we tried using GridBox for this. I'll open a scenery issue.

samreid commented 1 year ago

@zepumph and I made a commit above to correct the problem, and discussed with @jonathanolson to open a side issue for future improvement. @KatieWoe can you please review this in master? It currently looks like this for us:

image

If this looks good, please reassign to me and label as ready for cherry-picking.

KatieWoe commented 1 year ago

This looks good on master with stringTest=long

KatieWoe commented 1 year ago

This does not seem to be fixed in https://github.com/phetsims/qa/issues/841

samreid commented 1 year ago

I see this in RC.3, do you see something different?

image
KatieWoe commented 1 year ago

Just wen to take a screenshot and realized the window was partly off screen, which probably caused what I was seeing. It still looks lower than it did though, even in your screenshot.

zepumph commented 1 year ago

It is different code so it looks slightly different, I don't think the previous version was ever actually centered, it just ensured a bit of margin on both the top and bottom, as we are doing here. Does the current version seem acceptable given that consideration?

KatieWoe commented 1 year ago

I think so. Thanks for clarifying. Closing for now, is something else comes up I'll reopen.

KatieWoe commented 1 year ago

Reopening. During https://github.com/phetsims/qa/issues/844 I noticed that, when the strings are long, the icons change position and drop down, and if only one is long, the icons are not aligned with each other. This happens in Gravity and Orbits as well. longshift This is what the icons should look like: normal

samreid commented 1 year ago

I believe when we discussed with @jonathanolson and @zepumph we raised the idea of a grid layout but decided against it. @arouinfar how severe would you consider this problem and how much effort should be made to align those for varying string lengths?

arouinfar commented 1 year ago

I am not particularly bothered by the vertical shift. However, I find it odd that the first screen's maxWidth appears to be so much smaller than the second screen. Switching the localeProperty while the sim is running has a different appearance than loading the sim with the locale query parameter. Here's an example with locale=bs where the screen names have a similar character count.

Switching the localeProperty at runtime:

image

Initializing the locale with a query parameter:

image

It seems like the culprit may be the relative size of the English strings. "Model" is much shorter than "Real Molecules", so the first screen ends up with less width when dynamically changing the locale. Is this the expected behavior? It doesn't seem ideal to me, but I wouldn't consider it a publication-blocking issue, either.

image

Back to @jonathanolson and @samreid.

Edit: screenshots were taken from the latest dev of Molecule Shapes (currently in QA testing) https://phet-dev.colorado.edu/html/molecule-shapes/1.5.0-dev.4/phet/molecule-shapes_all_phet.html

samreid commented 1 year ago

It looks like in issue https://github.com/phetsims/joist/issues/438 we added this code:

https://github.com/phetsims/joist/blob/5cf0f5d641eda304b104668444cbd61cf2611b5b/js/NavigationBarScreenButton.ts#L228-L238

This means the maxWidth of the screen icon is being set to its initial width. Should we just choose a constant for this, or perhaps a function that only depends on the number of screens?

arouinfar commented 1 year ago

Thanks @samreid! I opened https://github.com/phetsims/joist/issues/884 to continue the discussion around screen title maxWidth. I don't think anything else remains for this issue, so I'll close.