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

plots: upgrade to latest Vega library versions #6371

Merged
merged 1 commit into from
Aug 16, 2021
Merged

plots: upgrade to latest Vega library versions #6371

merged 1 commit into from
Aug 16, 2021

Conversation

arjvik
Copy link
Contributor

@arjvik arjvik commented Jul 31, 2021

  • ❗ I have followed the Contributing to DVC checklist.

  • [N/A] πŸ“– If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

The current version of Vega included in the plots template page is quite outdated. Many of the features mentioned in the Vega Lite documentation are Vega-Lite 5.0 only, and thus fail to work on the 4.0 version included by DVC. I submitted this change that simply changes the version numbers to the latest versions released as of now.

One idea to consider is omitting the version entirely and allowing jsDelivr to pick the latest version it has available. Vega plots all require a $schema parameter that specifies the version of the Vega specification to adhere to when displaying, so even breaking changes to the latest spec should not cause issues to existing plots.

@arjvik arjvik requested a review from a team as a code owner July 31, 2021 22:14
@arjvik arjvik requested a review from isidentical July 31, 2021 22:14
@isidentical isidentical requested a review from pared July 31, 2021 23:40
@pmrowla
Copy link
Contributor

pmrowla commented Aug 1, 2021

related: #6218

as noted in that issue, if you need specific features you can define your own HTML template to override vega-lite version used in the default DVC templates

@arjvik
Copy link
Contributor Author

arjvik commented Aug 1, 2021

@pmrowla Thanks for the suggestion, that's exactly what I'm doing right now.

Copy link
Contributor

@pared pared left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have manually checked the current existing templates. They seem to be working fine.

@pared
Copy link
Contributor

pared commented Aug 10, 2021

@arjvik can you rebase on current master?

@arjvik
Copy link
Contributor Author

arjvik commented Aug 10, 2021

@pared done!

@arjvik
Copy link
Contributor Author

arjvik commented Aug 13, 2021

@pared Seems like both automated and manual tests have passed... is this going to get merged?

@pared
Copy link
Contributor

pared commented Aug 16, 2021

@arjvik sorry for the delay, yes I think we can merge it.

@pared pared merged commit 41bc819 into iterative:master Aug 16, 2021
@efiop efiop added the enhancement Enhances DVC label Aug 16, 2021
@jorgeorpinel
Copy link
Contributor

Hi @pared should we do a super quick update to the example in https://dvc.org/doc/command-reference/plots#example-offline-html-template ? Also, that should be an H2 😁 (it's an H3 rn). Thanks

@pared
Copy link
Contributor

pared commented Sep 6, 2021

@jorgeorpinel lets continue in iterative/dvc.org#2798

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhances DVC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants