search

phetsims / curve-fitting

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

lots of duplication in BarometerX2Node and BarometerR2Node #120

Closed pixelzoom closed 5 years ago

pixelzoom commented 5 years ago

There is lots of duplication in BarometerX2Node and BarometerR2Node, see TODO comments in the code.

There's also some weirdness in BarometerX2Node that makes it necessary to define OFFSET and use a VStrut. I suspect that this is unnecessary, and is a workaround for some other problem that wasn't addressed properly.

In any case, it seems like a BarometerNode base class should be factored out.

SaurabhTotey commented 5 years ago

As of right now, I have deduplicated a lot of the code between BarometerR2Node and BarometerX2Node by making a common BarometerNode superclass. However, as for the VStrut with the OFFSET in BarometerX2Node, this variable seems to be a workaround for spacing and placement of the BarometerX2Node. I haven't yet gotten around to fixing this, but I intend to put all of the alignment code in DeviationsAccordionBox rather than having BarometerX2Node take up extra invisible space. This should render the VStrut unnecessary and allow me to remove it.

SaurabhTotey commented 5 years ago

After committing 607397d0c9941cbb6a6a4d4b33d79c4ff695a602 which reworked DeviationsAccordionBox alignment, I was able to remove the VStrut in BarometerX2Node because it now doesn't contribute to alignment.