livebook-dev / kino_vega_lite

Vega-Lite (graphics) integration for Livebook
Apache License 2.0
26 stars 8 forks source link

Support updating of vega-lite parameters #45

Closed awerment closed 1 year ago

awerment commented 1 year ago

Decided to take a stab at a solution for the follow-up question from the Elixir Forum.

This adds a Kino.VegaLite.signal/3 function to send signals to the underlying chart.

Signal support in vega-lite seems to be spotty (see discussion here) and there is a naming mismatch to livebook-dev/vega_lite, as the registration is done via the VegaLite.param/3 function, which was probably named to match the naming in the official vega-lite implementation for dynamic charts , but I feel this is a dead-simple addition and a good starting point to extend the support for dynamic charts.

awerment commented 1 year ago

@bmitc Hello again :) Not sure how this PR will fare, but I thought you might be interested in any case.

jonatanklosko commented 1 year ago

Hey @awerment! Thanks for looking into this. As far as I understand, signals are not part of the official vega-lite API. There is no documentation on this and more importantly the JSON schema doesn't support signal, so using {"signal": "name"} is technically not valid vega-lite. It surely looks useful, but unfortunately I think we shouldn't rely on internals like that.

awerment commented 1 year ago

Hi @jonatanklosko, thanks for the quick feedback! Had the suspicion that the spotty/unofficial support might be a hindrance to adding this. For what it's worth, I think using {"expr": "name"} is documented and also (mostly) works. In VegaLite this would be:

VegaLite.new(width: 400, height: 400)
|> VegaLite.param("strokeWidth", value: 3])
|> VegaLite.mark(:line, stroke_width: [expr: "strokeWidth"])
...

Kino.VegaLite.signal/3 could be renamed to Kino.VegaLite.push_param/3 or similar to match this, but that would still leave the JS-side call to view.signal(), which could be confusing.

That said... from what I gather, the direction the vega-lite API will go in is not clear yet, so I'd totally understand if you say it's better to revisit this at a later time.

jonatanklosko commented 1 year ago

@awerment oh this sounds good, this section outlines this behaviour and we can link to that. I would make it Kino.VegaLite.set_param/3 just to separate from data push/push_many.

awerment commented 1 year ago

Cool! Updated :)

awerment commented 1 year ago

@jonatanklosko Thank you for the review, merge and very pleasant first OSS contrib. experience. 😃 ❤️