phetsims / natural-selection

"Natural Selection" is an educational simulation in HTML5, by PhET Interactive Simulations
GNU General Public License v3.0
3 stars 7 forks source link

fix options handling in PopulationGridNode.VerticalLines #229

Closed pixelzoom closed 4 years ago

pixelzoom commented 4 years ago

Related to #203 (code review) @jonathanolson had this REVIEW comment about PopulationGridNode.VerticalLines:

class VerticalLines extends Node {
...
  constructor( xRangeProperty, options ) {
...
    const path = new Path( shape, options );
...
    super( options ); //REVIEW: We're passing the options both to the VerticalLines AND its Path? And clipArea is applied to both?
  }
...
}

Good catch, this is not what I intended here. Passing all options to a subcomponent is (imo) an anti-pattern. And clipArea makes it worse here. I will address, probably via nested options for the Path.

pixelzoom commented 4 years ago

Fixed in above commit, using nested option pathOptions for the Path subcomponent of VerticalLines.

@jonathanolson please review.

pixelzoom commented 4 years ago

I'll be taking shas for the 1.2 branch on 9/17, and this needs to be reviewed before then. So high priority please.

jonathanolson commented 4 years ago

Looks great to me, thanks!