-
Notifications
You must be signed in to change notification settings - Fork 623
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
feat: frame animations with time encoding and timer param #8921
Conversation
Thanks. Can we see whether you can run the formatting action as well so we know the formatting doesn't break. |
i was able to run |
Great. I was hoping we could get the GitHub action for checking formatting to work somehow. |
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.
Sorry for the delay, y'all! A great start. I've left line-level comments throughout and will do a more holistic review next. In the meantime, some higher-level thoughts:
- Thanks for including example specs in your PR OP. Could you include them as json specs under
examples
as well please? That'll slurp them into our CI process (and, I think?, make them easier to expose in the documentation) - It might behoove us to also include some runtime tests since they've saved our (namely, my) behind a number of times when the compile-time specs appeared to be correctly constructed? (I'm happy to walk through the runtime test infrastructure post-CHI since it's a little complicated).
first round of comments has been addressed. pending todos:
|
Hi @jonathanzong and @joshpoll!👋 Would you mind updating this PR so the latest changes from the main repository are also included in this branch? Im not brave enough to do it myself🙈, but I'm sensing that this is the reason why the new deployment preview is not yet triggering. |
Just thinking about making a map with the trajectory of the upcoming eclipse🥳 |
Such a cool idea! Please share if you create it, I would love to see an animated VL chart for this. Btw, @jonathanzong are you waiting for a review on this branch or are you planning to add more commits (I saw you were still adding more since you last requested a review). |
I rebased |
We are indeed just waiting for a review |
Yes, apologies, it's been on my docket for a while but I've been underwater with various other deadlines. My plan is to wrap this up this month 🤞 |
81b135c
to
488dece
Compare
i have rebased, fixed checks, and verified that the animation works as expected in local editor edit: i'm going to wait to fix the codecov until after we get a review because otherwise we'll just have to do it again |
"mark": "point", | ||
"params": [ | ||
{ | ||
"name": "avl", |
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.
Minor point but it'd be great to name this in a more semantically-meaningful way vs (what I assume) is an abbreviation of "Animated Vega-Lite".
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 have added an error explaining that facet, layer, and concat animations are unsupported when a user tries to create them (and unit tests for this error). how serious are we about codecov? if you look at what's left, it's codecov asking for:
can we just bypass this? |
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're good to go here. And I agree, fine to bypass the codecov—it seems to be failing on exceedingly minor things.
Follow up from #8921 --------- Co-authored-by: GitHub Actions Bot <[email protected]>
@jonathanzong @arvind this was missing from #8921 --------- Co-authored-by: GitHub Actions Bot <[email protected]>
This change implements basic features of Animated Vega-Lite. With this change, users can create frame animations using a
time
encoding, atimer
point selection, and afilter
transform.This change does not include more complex features e.g.: interpolation, custom predicates, rescale, interactive sliders, or data-driven pausing.
time
encoding channelisTimerSelection
function to check if a selection is an animation selections_curr
animation dataset fortimer
selections to store the current animation frameanim_clock
), current animation value (anim_value
), current position in the animation field's domain (t_index
), etc.time
encoding is present, updates associated marks'from.data
to use the animation dataset (current frame).Relevant issue: #4060
Coauthor: @joshpoll
Example specs
Hop example:
Gapminder: