search

phetsims / wave-interference

"Wave Interference" is an educational simulation in HTML5, by PhET Interactive Simulations.
MIT License
19 stars 5 forks source link

NumberControls are vertically mis-aligned when maxWidth restricts their heights differently #422

Closed samreid closed 5 years ago

samreid commented 5 years ago

From https://github.com/phetsims/wave-interference/issues/418#issuecomment-518733487

With longer-than-english strings, the heights of boxes are different and the vertical alignment can be off by a little bit:

image

@arouinfar or @ariel-phet do you want to spend any time on this before publication? Also tagging @KatieWoe so she's aware of this problem.

UPDATE: Linking back to https://github.com/phetsims/QA/issues/389

ariel-phet commented 5 years ago

@samreid given that this is a common code component that recently got work, yes, it seems we should spend some time as this would be a general issue I think (probably not noticed before since we often do not have multiple number controls in a horizontal row like this) but I believe some planned components in CCK AC have this arrangement, so best to deal with sooner than later.

samreid commented 5 years ago

@jonathanolson can you suggest an implementation strategy for this problem?

samreid commented 5 years ago

Two possibilities I considered:

  1. Use AlignBox to track component sizes and make sure they match up. This would lead to very complicated client-side code and require knowledge of all the layouts of different NumberControls and/or requirement for client to selectively pass AlignBoxes to the right places.
  2. Reserve a vertical "footprint" shrinkable parts of NumberControl via struts or equivalent, based on the unscaled height of components. But do we want to always do this, or make it an option?

Also, it seems unlikely this problem is isolated to NumberControl.

I'd be comfortable applying solution (2) to Wave Interference, but this issue would feel incomplete without discussion with @jonathanolson and/or a discussion of how this problem may manifest and be addressed elsewhere. This is one of the last issues before Wave Interference can go to RC, so I'll elevate the priority.

samreid commented 5 years ago

At today's meeting @pixelzoom suggested aligning to the bottom. I asked if that was general enough, @ariel-phet said it is acceptable. I'll proceed.

samreid commented 5 years ago

This looks acceptable to me, I'll commit

image

samreid commented 5 years ago

Fixed in commit, @arouinfar or @ariel-phet or @KatieWoe can you please review? Close if all is well.

arouinfar commented 5 years ago

Looks good to me @samreid

samreid commented 5 years ago

Thanks @arouinfar, closing.