-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Data module #49
Data module #49
Conversation
```elixir | ||
# A line plot with point: true using the shorthand api | ||
Data.chart(fuels, [type: :line, point: true], x: "total", y: "solid_fuel") | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here the Data
example comes last, but in the previous example the Data
example comes first. Should we make it consistent?
# Piping the shorthand api into a enconde_field | ||
Vl.new(width: 500, height: 300, title: "Fuels") | ||
|> Data.chart(fuels, :point, x: "total", y: "solid_fuel") | ||
|> Vl.encode_field(:color, "total", type: :quantitative, scale: [scheme: "category10"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an issue in this approach. This code only works because both x
and color
use "total". But if the color was encoded with another value, then its field would not be included in only
. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! We rely on Table.Reader
, so it might be better to remove the implicit only
and allow passing it as an option of the data
argument. Or maybe make it implicit by default and use the option to override it. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can have one option called additional_fields or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer only
for consistency, like in values_from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Data.chart([data: fuels, only: …], :point, x: "total", y: "solid_fuel")
And that should be the only argument accepted for data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, you said nothing about having or not a default…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a default, for sure. But I was wondering if we could have that as part of fields somehow:
Data.chart(fuels, :point, x: "total", y: "solid_fuel", additional_fields: [:foo, :bar])
Another idea is to move the logic VegaLite
itself. We could support only: :lazy
and then we compute it the time we build the JSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge this without tackling this problem and please open up an issue so we do it next. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok!
Co-authored-by: José Valim <jose.valim@gmail.com>
Co-authored-by: José Valim <jose.valim@gmail.com>
Co-authored-by: José Valim <jose.valim@gmail.com>
Co-authored-by: José Valim <jose.valim@gmail.com>
Co-authored-by: José Valim <jose.valim@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some final nitpicks. We can handle the :only
separately. :)
Co-authored-by: José Valim <jose.valim@gmail.com>
Co-authored-by: José Valim <jose.valim@gmail.com>
Co-authored-by: José Valim <jose.valim@gmail.com>
Short hand api and specialized charts