-
Notifications
You must be signed in to change notification settings - Fork 567
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
Clarify that reveal.js 3.x is needed #735
Conversation
+1 as I said in the issue. |
Looks good but I still think that the default of using the cdn (#732) is a better default for most people… unless we want to explicitly help people download an up-to-date version of reveal.js. My intuition is that we should provide that utility with something like a |
Actually can you add a reference to the latest example of reveal.js that works? Also a test to make sure that the version mentioned in the docs works? That way, we’ll know if it ever breaks with the a nbconvert change? |
I've added a note to the docs showing how to get reveal 3.6. Adding a test that reveal.js works would be a big job, however - it involves executing Javascript in a browser, which the nbconvert tests are not set up to do. And we know from the notebook tests what a pain it is. |
docs/source/usage.rst
Outdated
|
||
git clone https://github.com/hakimel/reveal.js.git | ||
cd reveal.js | ||
git checkout 3.6.0 |
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.
We are using reveal.js 3.5.0 in the serve post-processor, we should keep it consistent and checkout 3.5.0 until we decide to upgrade the whole thing to 3.6.0, I think.
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.
Thanks Damian, updated.
Closing since we now have #732 merged with this inside of it. |
Closes gh-702