phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

AccordionBox({stroke:null}) fails #678

Closed samreid closed 3 years ago

samreid commented 3 years ago

Discovered during https://github.com/phetsims/sun/issues/676, if you pass stroke: null to AccordionBox, you get this error:

AccordionBox.js:398 Uncaught TypeError: Cannot set property 'rectWidth' of undefined
    at AverageSpeedAccordionBox.layout (AccordionBox.js:398)
    at new AccordionBox (AccordionBox.js:320)
    at new AverageSpeedAccordionBox (AverageSpeedAccordionBox.js:109)
    at new EnergyScreenView (EnergyScreenView.js:43)
    at EnergyScreen.createView (EnergyScreen.js:26)
    at EnergyScreen.initializeView (Screen.js:252)
    at Array.<anonymous> (Sim.js:884)
    at Sim.js:892

Assigning to @jbphet and @pixelzoom who are listed as authors in AccordionBox.

pixelzoom commented 3 years ago

@samreid Please clarify. Do you have a use case where you want to be able to set stroke: null? Or is it sufficient to add an assertion that requires stroke to be non-null?

samreid commented 3 years ago

I don't have a use case that requires a null-stroked Accordion box. But the options look like so:

      // box
      stroke: 'black',

and there is no indication that null is disallowed. I don't have a preference about whether we add support for null or add an assertion that it is non-null.

pixelzoom commented 3 years ago

Fixed in the above commit, AccordionBox now supported stroke: null.

this.expandedBoxOutline and this.collapsedBoxOutline are created conditionally, if options.stroke is truthy. So in layout, they should be updated only if they exist.

@samreid please review.

samreid commented 3 years ago

I reviewed the commit and tested with null in build an atom, everything seems nice! Closing.