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

RulerNode: unitsLabelMaxWidth doesn't support changing label with PhET-iO #527

Closed
zepumph opened this issue Aug 22, 2019 · 2 comments
Closed

Comments

@zepumph
Copy link
Member

zepumph commented Aug 22, 2019

From phetsims/gravity-force-lab#76, I noticed that changing "meters" to something longer on the RulerNode in GFL studio overlaps with the first major tick label.

Looking at the implementation of RulerNode, it wasn't obvious to me how to support this. I saw that @jbphet had made some commits in the area about scaling the units text. @jbphet does this seem like something that would be straight forward for you to add?

Steps to reproduce the text overlap:

  • phetmarks
  • gravity-force-lab studio
  • On the right side, PhET-iO Elements set to All
  • navigate to gravityForceLab.gravityForceLabScreen.view.ruler.unitsLabel.textProperty
  • type something longer in the text, and see the overlap

image

This may be an issue with non PhET-iO setting of the units, but I don't really know.

@jbphet
Copy link
Contributor

jbphet commented Aug 23, 2019

The code was written to only adjust the max width of the units label if necessary at load time. Since it is now possible to change the label through PhET-iO after load time, I changed the code to always set the max width of the units node. This seems to work, and I tested it by running with stringTest set to long, double, x, and to nothing (i.e. the normal case), and it looked fine in all cases. I also tested the nominal case in Masses and Springs, since it uses a vertical ruler, and it looked okay there too.

It makes me a little nervous to change a widely used node in this way, so I request that @zepumph review the code change carefully and test it a bit more so that we can feel confident that this change doesn't break anything.

@zepumph
Copy link
Member Author

zepumph commented Aug 23, 2019

The commit looks clean and concise to me. No thoughts or worries there.

This works really well in the GFL case and in BLL when changing in studio. I then launched BLL in studio with stringTest=long, and I was able to shorten it and it re-scaled nicely. I also chose a random sim and launched it with stringTest=long in phet brand (FPAF). Things look really good! Thanks for the speedy change. I'm going to close this.

@zepumph zepumph closed this as completed Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants