phetsims / bamboo

Charting library built with Scenery
http://scenerystack.org/
MIT License
3 stars 0 forks source link

Create a plot that draws data with arrows #34

Closed jessegreenberg closed 3 years ago

jessegreenberg commented 3 years ago

From https://github.com/phetsims/greenhouse-effect/issues/44, we need a plot that is similar to BarPlot but where the bars have arrow heads. Over slack I checked to see if anyone had a recommendation for how to go about this:

Jesse Greenberg 5:16 PM I have a plot that looks like this: image.png image.png 5:17 Which is currently using BarPlot from Bamboo. But it has been requested that arrow heads be added to each bar, which BarPlot doesn't support. Does that seem like something BarPlot should provide or do you think I should roll my own? Chris Malley 5:18 PM Arrow heads are typically added to bars (in PhET sims) only when the value of the bar is off scale. So first, I would push back on the request — it’s non-standard, and likely to confuse. (edited) 5:20 I see that Excel supports “up down arrow” charts, https://www.youtube.com/watch?v=wRvyrznLDiA, so maybe that’s where this request is coming from. YouTubeYouTube | Karina Adcock How to make an up and down arrows chart in excel Jesse Greenberg 5:20 PM There is another graphic in the sim that looks like this, and the motivation for the request was to make the two look the same. image.png image.png Chris Malley 5:21 PM I would create UpDownArrowPlot, a new Plot type for bamboo. Jesse Greenberg 5:21 PM Arrows for energy flux make sense to me, I can't remember why arrows were needed to represent energy balance. Chris Malley 5:22 PM Or call it whatever you think is best. I suggested UpDownArrowPlot because Excel seems to call these “up down arrow charts”. Jesse Greenberg 5:22 PM OK, sounds good, thanks! Chris Malley 5:23 PM It’s API should be identical to BarPlot (and other Plot classes). That would be more obvious if JavaScript had interfaces. (edited) 5:24 Public methods are obvious. But note the public this.dataSet field.

To summarize, the recommendation was to create a new class that draws the data with arrows, called UpDownArrowPlot. The class should have the same API as all the other plots in bamboo.

jessegreenberg commented 3 years ago

I added an UpDownArrowPlot in the above commit. It is much like BarPlot, but draws the data with ArrowNode instead of Rectangle.

@samreid would you mind reviewing this addition? One thing I noticed was that WebStorm flagged some things in the constructor as duplicated code with BarPlot, which comes from them all having a matching API. Do you think it would be worthwhile to have a supertype like BamboPlot that is extended by BarPlot, UpDownArrowPlot, Lineplot, CanvasLinePlot (and maybe others) that implements these kinds of lines?

image

(Green highlight is what WebStorm flagged as duplicated).

jessegreenberg commented 3 years ago

A better way to add/remove the graphic for each data point was shown in #33. Rather than removing all then adding all back each update, it should just add/remove the new ArrowNodes if necessary. This improvement was made in the following commit.

samreid commented 3 years ago

The changes look good and I tested them in greenhouse effect. Would you like to add a bamboo demo that shows the UpDownArrowPlot?

I also opened https://github.com/phetsims/bamboo/issues/35 to factor out a BambooPlot parent type.

jessegreenberg commented 3 years ago

OK sounds good, thanks for reviewing. I made a simple demo for the BambooDemoScreenView. Anything else for this one?

samreid commented 3 years ago

Looks great, thanks! Closing.