phetsims / griddle

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

"axis" vs "orientation" in bamboo API #89

Closed pixelzoom closed 3 years ago

pixelzoom commented 3 years ago

Throughout the bamboo API, parameter {Orientation} orientation is confusing. It means the orientation of the axis that the component is associated with, not the orientation of the component itself. So changing orientation to axis would be clearer. Using {Orientation} to mean "which axis" also seems wrong/confusing, so a new enum seems needed.

Slack discussion Chris Malley 3:02 PM For this TODO in GridLineSet, yes, it's currently backward. I made a quick (failed) attempt at fixing it. ```js @param {Orientation} orientation - if the lines themselves are HORIZONTAL or VERTICAL. TODO: is this backwards? ``` Sam Reid 3:04 PM I thought something looked suspicious. Sam Reid 3:12 PM One thing I struggled with is: the horizontal lines which are drawn at different vertical values. Should they be called Horizontal or Vertical? Chris Malley 3:14 PM Well, GridLineSet orientation is documented as the orientation of "the lines themselves", which I think is most intuitive. You could just change the doc. But I would tend to think of "horizontal lines" vs "lines for the vertical axis". Sam Reid 3:15 PM Makes sense Chris Malley 3:15 PM Orientation enum defines `opposite`, if that helps. So I think we want to specify the orientation of lines themselves, then use orientation.opposite for the MVT that determines where the lines occur. Sam Reid 3:19 PM I mostly agree, but TickMarkSet seems to be an exception. Say we are talking tick marks along the y-axis. The tick marks are horizontal, but it seems preferable to call them vertical since they are on the vertical axis. Chris Malley 3:20 PM I think of tick marks as which axis they are associated with, not the orientation of the ticks themselves. Similarly for LabelSet. I can live with GridLineSet orientation also meaning "the axis they are associated with". In that case, `axis` may be a better param name than `orientation`. ```js @param {Orientation} axis - the axis that the gridlines (tick marks, labels, ...) are associated with ``` Using `{Orientation}` here to mean "which axis" also feels a little wrong/confusing. Sam Reid 3:26 PM I agree
pixelzoom commented 3 years ago

How about just changing {Orientation} orientation to {Orientation} axisOrientation throughout? That should clarify, and {Orientation} then seems appropriate.

samreid commented 3 years ago

Fixed in the commits. No changes to Fourier were necessary because of the other "Todo: is this backwards" bug I fixed at the same time. Ready for review. Close if all is well.

pixelzoom commented 3 years ago

👍🏻 Closing.