phetsims / color-vision

"Color Vision" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/color-vision
GNU General Public License v3.0
1 stars 7 forks source link

HeadNode argument is off by 15? #34

Closed samreid closed 10 years ago

samreid commented 10 years ago

I noticed this code in HeadNode:

  function HeadNode( image, bottom ) {

    Image.call( this, image,
      {
        bottom: bottom + 15,
        left: 50,
        scale: 0.7
      } );
  }

The 15 seemed odd to me. Why is 15 added to the parameter? Why not move the 15 to the call site(s)?

samreid commented 10 years ago

Also, this may be a good situation to move the "bottom" argument into the options parameter, and use {left:50, scale:0.7} as the defaults.

aaronsamuel137 commented 10 years ago

I added 15 here instead of the call sites because it is the same in every call site, and it seemed easier to change in one place instead of many.

The 15 is just a slight offset to make the bottom of the head image align with the bottom of the screen in a browser. It kept changing every time Bryce updated the head image, so I wanted to have it easy to change in one place.

The bottom argument is expecting layoutBounds.bottom.

Another option would be to create a constant for the head offset and pass it from every call site. What do you think is best in this case?

samreid commented 10 years ago

Perhaps adding documentation and explanation as in your above comment in HeadNode would solve the problem.

aaronsamuel137 commented 10 years ago

Added documentation to HeadNode. Assigning to @samreid for review

samreid commented 10 years ago

The docs and parameter renaming look great, thanks! Closing.