Skip to content
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

Make :field optional in vega lite spec #62

Merged
merged 2 commits into from Aug 14, 2021
Merged

Conversation

rfhayashi
Copy link
Contributor

:field can be omitted when doing things like histograms (e.g. https://vega.github.io/vega-lite/docs/bin.html#histogram). At the moment is possible to workaround this by setting :field to a random value, but it took me a long time to figure out why the chart didn't render. I'm not sure if :field should also be made optional for :x.

@rfhayashi
Copy link
Contributor Author

@BrianChevalier Could you review this?

@BrianChevalier
Copy link
Collaborator

@rfhayashi Sorry to hear about the lost time 😞 This seems like a reasonable change, but I'm not sure if it will cause false positives (trying to render Vega-lite when map is not actually Vega-lite). Ideally we would have comprehensive 'correct' specs for Vega-lite. @djblue thoughts on this?

@rfhayashi
Copy link
Contributor Author

@BrianChevalier This is an amazing thing that you did. I'm loving it. More than worth that lost time. My take is that if a map has :data, :mark and :enconding, it should be treated as vega and the viewer show an error if that is invalid. One can always change the viewer, so I guess that would be a good trade-off.

@djblue
Copy link
Owner

djblue commented Aug 14, 2021

@BrianChevalier thanks for the feedback and I think making things easier is reasonable. Thanks @rfhayashi for the PR!

@djblue djblue merged commit 8bc348f into djblue:master Aug 14, 2021
@rfhayashi rfhayashi deleted the patch-1 branch August 16, 2021 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants