search

phetsims / collision-lab

"Collision Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 4 forks source link

Branch: ball-vector2-properties #160

Closed brandonLi8 closed 4 years ago

brandonLi8 commented 4 years ago

For https://github.com/phetsims/collision-lab/issues/159

I'd also like a response on whether this is necessary to split into x/y Properties at this level. Can't this be on a view component so that it only modifies a specific component? Is there something from phet-io where we gain from having separate x/y? Having a single positionProperty and velocityProperty seems cleaner in general.

I looked over this again and it seems like we can remove having separate x/y properties. The only time these Properties are referenced in other classes is in BallValuesPanelNumberDisplay, which I suppose should display a DerivedProperty of the component of the position/velocity vector2Properties. When the user goes to edit the value via KeypadDialog, it seems like instead of passing in a Property to modify we could directly set one of the components, as you mentioned.

I'll investigate this locally.

It also seems that BallValuesPanelNumberDisplay could see some improvements, particularly the BallValuesPanelColumnTypes enumeration. I'll try converting this to a Rich Enumeration to remove the static methods in BallValuesPanelNumberDisplay.

brandonLi8 commented 4 years ago

In the commits above, I was able to remove the x/y component-specific Properties. I added setters for the specific components, instead.

I would like @jonathanolson to look at this branch. The files that were changed are Ball, KeypadDialog, BallValuesPanelNumberDisplay, and BallValuesPanelColumnTypes.

jonathanolson commented 4 years ago

Looks great! Merged to master.