phetsims / griddle

Dynamic charting library built with Scenery
MIT License
2 stars 4 forks source link

rename XYChartNode options #81

Closed pixelzoom closed 3 years ago

pixelzoom commented 3 years ago

While reviewing https://github.com/phetsims/fourier-making-waves/issues/8, I came across these options in XYChartNode:

      // Default ranges in model coordinates for the model-view transform - the chart will display
      // these ranges of data within the view dimensions of width and height specified above.
      // These have no impact if you provide your own modelViewTransformProperty.
      defaultModelXRange: new Range( 0, 4 ),
      defaultModelYRange: new Range( -1, 1 ),

      // {Node} - label for the vertical axis, should already be rotated if necessary
      verticalAxisLabelNode: null,

      // {Node} - label for the horizontal axis
      horizontalAxisLabelNode: null,

      // {number} - number of decimal places for the labels along grid lines
      verticalGridLabelNumberOfDecimalPlaces: 0,
      horizontalGridLabelNumberOfDecimalPlaces: 0,

      // line spacing, in model coordinates
      majorVerticalLineSpacing: 1,
      majorHorizontalLineSpacing: 1,

This class is named XYChartNode. So referring to "vertical" and "horizontal" is unnecessarily verbose, unless we plan to swap x & y (which I doubt). It's also inconsistent with options defaultModelXRange and defaultModelYRange. So recommended to replace "vertical" and "horizontal" with "x" and "y" throughout.

"labels along the grid lines" are typically called "tick mark labels", or more tersely "tick labels". So recommended to rename the overly-verbose verticalGridLabelNumberOfDecimalPlaces and horizontalGridLabelNumberOfDecimalPlaces to:

      // {number} - number of decimal places in tick labels
      xTickLabelDecimalPlaces: 0,
      yTickLabelDecimalPlaces: 0,
pixelzoom commented 3 years ago

While we're on the topic of XYChartNode options:

      // {Object|null} - Options for the Rectangle that contains chart content, including GridNode and
      // DynamicSeriesNodes.
      chartPanelOptions: null, // filled in below because some defaults are based on other options

There's no Panel in XYChartNode, so this name is misleading. If it's a Rectangle, maybe call it backgroundRectangleOptions.

samreid commented 3 years ago

As far as I can tell, this is less problematic in bamboo. @pixelzoom can this issue be closed?

pixelzoom commented 3 years ago

Yes, addressed by bamboo. No need to address in deprecated (griddle) code. Closing.