Skip to content

Adding animation directive for injecting animated diagrams #1698

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

Merged
merged 3 commits into from
May 24, 2018

Conversation

gspencergoog
Copy link
Collaborator

This PR adds an animation directive that allows embedding of animations as MPEG 4 videos. The idea is to allow animated diagrams to be placed in documentation.

It places a play button over the video until clicked, at which point it hides the play button and plays the video until clicked again, when it pauses and the play button reappears.

The syntax for adding an animation is:

{@animation name 100 200 http://host.com/path/to/video.mp4}

Where the name is a unique name that is used for the id of the video tag, and the numbers are the width and height of the diagram in pixels.

@googlebot googlebot added the cla: yes Google CLA check succeeded. label May 23, 2018
@gspencergoog gspencergoog force-pushed the animation_directive branch from ac9baed to 96ff478 Compare May 23, 2018 00:11
@kevmoo kevmoo requested a review from jcollins-g May 23, 2018 03:39
@gspencergoog gspencergoog force-pushed the animation_directive branch from 96ff478 to 3a4dae3 Compare May 23, 2018 20:01
@gspencergoog gspencergoog force-pushed the animation_directive branch from 3a4dae3 to 6a1a19d Compare May 23, 2018 20:09
Copy link
Contributor

@jcollins-g jcollins-g left a comment

Choose a reason for hiding this comment

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

LGTM with a suggestion for improved tests

@@ -355,6 +355,37 @@ class Dog implements Cat, E {
/// Don't define this: {@macro ThatDoesNotExist}
void withUndefinedMacro() {}

/// Animation method
Copy link
Contributor

@jcollins-g jcollins-g May 23, 2018

Choose a reason for hiding this comment

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

Another interesting case would be a doc comment that has an animation on the first line:

/// Hello animation world:  {@animation stuffAnimation 100 100 http://etc}

Basically, to exercise what happens when an animation might appear in a oneLineDoc (the summary of each method displayed on a Class page, for example). It's probably desirable not to have the animation appear in the summary of each method, and I think that's what may happen, but a test should help.

And other cases where @animation is inlined:

/// A test
/// This is something cool {@animation a 100 200 http://stuff} that I am inlining.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, I've added tests for both inlined animations and oneLineDoc substitution. It seems that the oneLineDoc doesn't incorporate the animations in the first place (probably because the animation code is preceded by a blank line), so no problem there, and the inline test found a problem (I was indenting the first line after the animation code, which made markdown think that it was code), which I fixed.

@gspencergoog
Copy link
Collaborator Author

OK, I think this is ready to be committed. I don't have write access, so you'll need to commit. Thanks for the review!

@jcollins-g
Copy link
Contributor

Merging now. You just missed 0.19.1 but as preview-dart-2 gets closer to switching on by default I'll be rolling dartdoc frequently. You can expect a release with this feature sometime next week I hope.

@jcollins-g jcollins-g merged commit d834d1a into dart-lang:master May 24, 2018
@gspencergoog
Copy link
Collaborator Author

Great, I'll look for it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Google CLA check succeeded.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants