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

Strings may not be vertically centered #448

Closed
Tracked by #841 ...
KatieWoe opened this issue Sep 28, 2022 · 13 comments
Closed
Tracked by #841 ...

Strings may not be vertically centered #448

KatieWoe opened this issue Sep 28, 2022 · 13 comments
Assignees
Labels

Comments

@KatieWoe
Copy link
Contributor

Test device
Dell
Operating System
Win 11
Browser
Chrome
Problem description
For phetsims/qa#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 phetsims/joist#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
Copy link
Member

samreid commented Oct 4, 2022

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
Copy link
Member

samreid commented Oct 4, 2022

@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
Copy link
Contributor Author

KatieWoe commented Oct 4, 2022

This looks good on master with stringTest=long

@KatieWoe
Copy link
Contributor Author

KatieWoe commented Oct 6, 2022

This does not seem to be fixed in phetsims/qa#841

@samreid
Copy link
Member

samreid commented Oct 6, 2022

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

image

@KatieWoe
Copy link
Contributor Author

KatieWoe commented Oct 6, 2022

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
Copy link
Member

zepumph commented Oct 6, 2022

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
Copy link
Contributor Author

KatieWoe commented Oct 6, 2022

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

@KatieWoe KatieWoe closed this as completed Oct 6, 2022
@KatieWoe
Copy link
Contributor Author

KatieWoe commented Oct 21, 2022

Reopening. During phetsims/qa#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
Copy link
Member

samreid commented Oct 26, 2022

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?

@samreid samreid assigned arouinfar and unassigned jonathanolson and samreid Oct 26, 2022
@arouinfar
Copy link
Contributor

arouinfar commented Oct 26, 2022

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
Copy link
Member

samreid commented Nov 18, 2022

It looks like in issue phetsims/joist#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
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants