search

phetsims / forces-and-motion-basics

"Forces and Motion: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/forces-and-motion-basics
GNU General Public License v3.0
7 stars 10 forks source link

Use VerticalCheckBoxGroup in MotionControlPanel #258

Closed samreid closed 6 years ago

samreid commented 6 years ago

Discovered in https://github.com/phetsims/sun/issues/344, Forces and Motion: Basics is using VerticalCheckBoxGroup in NetForceControlPanel but not MotionControlPanel. Here's a picture of MotionControlPanel:

image

Note this means that MotionControlPanel: (a) doesn't have the right mouseArea for the checkboxes (should extend all the way to the right, like in the NetForceControlPanel) (b) duplicates and deviates from common code functionality (c) has an inconsistent implementation compared to NetForceControlPanel

Assigning to @ariel-phet for prioritization/timeline/delegation.

ariel-phet commented 6 years ago

@zepumph is this something you could handle? Seems like it would be beneficial for both iO and a11y, and @DianaTavares plans to use FAMB for the next dashboard study, so seems in your wheelhouse.

zepumph commented 6 years ago

I'll try to crank this out on thursday.

zepumph commented 6 years ago

I was able to convert this and it worked easily, simplifying the code a bit too.

Note that I dropped support for the instrumented enabledProperty in MotionScreenView in favor of a general solution to be implemented in https://github.com/phetsims/sun/issues/387.

@ariel-phet please assign a reviewer.

samreid commented 6 years ago

May I please review this change? I have familiarity with FAMB as well as VerticalCheckboxGroup. @ariel-phet please assign me if that seems reasonable.

ariel-phet commented 6 years ago

All you @samreid

zepumph commented 6 years ago

Like stated in https://github.com/phetsims/sun/issues/387#issuecomment-418797525, we may want to make sure that we are using the new default enabledProperty for Checkbox in this control panel.

samreid commented 6 years ago

Code change looks great and I tested that everything's working nicely. The new version also has the touch areas extend to the right (for short texts), which seems like an improvement.

UPDATE: For the sake of completeness, I did observe a minor layout change between this version and the published version in the size of the Net Force screen control panel:

Published image

Proposed image

However, this may have been due to a preceding change, and in any case the newer version doesn't look worse than the older version.