Skip to content

Conversation

@kolibril13
Copy link
Member

@kolibril13 kolibril13 commented Feb 14, 2021

As there are some example .py scenes, I think a notebook example is a good addition.
image

Depends on #1013

@kolibril13 kolibril13 added the pr:easy review There is nothing particular (i.e, it's about a general/small thing) to know for review! label Feb 14, 2021
Copy link
Member

@behackl behackl left a comment

Choose a reason for hiding this comment

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

I'm fine with adding an example notebook, but I'd strongly suggest not including cell output when doing so.

@kolibril13
Copy link
Member Author

yep, good point, I will remove it.

@kolibril13
Copy link
Member Author

As it is convenient to store jupyter notebooks in the manim project folder, I added the jupyter/ folder to the gitignore which ensures that the notebooks are not version controlled.

@naveen521kk
Copy link
Member

Can this be merged @behackl ?

@jsonvillanueva
Copy link
Member

With #1013, the syntax for jupyter notebooks would change and the sample provided would end up failing. I'm not sure how to test that the notebooks runs without error, but a test case that can be run with pytest would be helpful to ensure the notebook runs properly.

@behackl behackl dismissed their stale review March 21, 2021 10:10

Output has been removed.

@kolibril13
Copy link
Member Author

Here is how the binder link looks now in the docs: https://manimce--1029.org.readthedocs.build/en/1029/installation.html

Copy link
Member

@behackl behackl left a comment

Choose a reason for hiding this comment

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

Two more suggestions, and one further recommendation: I think it would be good if the example notebook also made use of the newly introduced config.media_width option. A new cell containing something like

# set the maximum width for video outputs to a predefined value
config.media_width = "42%"

should be enough. Then this is (finally) ready to merge, from my point of view.

@kolibril13
Copy link
Member Author

Depends on #1013.
I just tried it, and without #1013 this is not working currently:
image

@github-actions github-actions bot added the pr:dependent This PR or issue requires that another PR is merged first label Mar 24, 2021
@jsonvillanueva
Copy link
Member

I'm generally unsure what the appropriate label for this PR is. Maybe documentation?

@github-actions github-actions bot removed the pr:dependent This PR or issue requires that another PR is merged first label Apr 2, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Apr 2, 2021

🎉 Great news! Looks like all the dependencies have been resolved:

💡 To add or remove a dependency please update this issue/PR description.

Brought to you by Dependent Issues (:robot: ). Happy coding!

@kolibril13 kolibril13 requested a review from behackl April 2, 2021 16:06
@jsonvillanueva jsonvillanueva added the documentation Improvements or additions to documentation label Apr 2, 2021
Copy link
Member

@behackl behackl left a comment

Choose a reason for hiding this comment

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

Resolved the merge conflict and tested locally: LGTM.

(The banner animation seems to be currently broken though, which is unfortunate.)

@behackl behackl merged commit 2b6b2cd into master Apr 8, 2021
@behackl behackl deleted the example_notebook branch April 8, 2021 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation pr:easy review There is nothing particular (i.e, it's about a general/small thing) to know for review!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants