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

Locally hosted reveal.js leads to blank page when serving without --post #702

Closed
fperez opened this issue Nov 8, 2017 · 9 comments
Closed
Labels
docs format:Slides pertains to exporting to the Slides format

Comments

@fperez
Copy link
Member

fperez commented Nov 8, 2017

When converting to slides and serving with --post serve, things work fine. But if the same exact HTML file is then served locally, even with a local copy of reveal.js available:

alpamayo[jslides]> d
/Users/fperez/dev/jslides
total 704
drwxr-xr-x@ 11 fperez     374 Nov 10  2012 reveal.js/
-rw-r--r--   1 fperez   51552 Oct 27 16:26 simple-test.ipynb
-rw-r--r--   1 fperez  304586 Nov  7 18:12 simple-test.slides.html

the resulting HTML page is blank, with the following error on the JS console:

image

In a comment to issue 91, an explicit solution to this problem is provided, and I tested it works: adding the option --reveal-prefix "https://cdnjs.cloudflare.com/ajax/libs/reveal.js/3.1.0". But as mentioned in #296, we want to make it possible to easily serve slides fully offline too, so this workaround (while useful for some cases) isn't a full solution.

Additionally, the current instructions say that having reveal.js in the current directory works, which at least in my case (and confirmed by @Carreau and @pbstark, who originally reported the problem), it doesn't.

So I think we need to:

  • Improve the instructions to clarify exactly what to do for a locally hosted reveal.js, that actually works (I tried multiple flavors of --reveal-prefix with ., ./, ./reveal.js, and none worked).

  • Also provide the explicit tip of using the CDN path (--reveal-prefix "https://cdnjs.cloudflare.com/ajax/libs/reveal.js/3.1.0"), which is confirmed to work and for some cases works. I can make a PR for this if it's confirmed it's the right thing to do.

  • Figure out fully offline hosting with clear instructions (essentially blank page when using --to slides offline #296).

@fperez fperez added docs format:Slides pertains to exporting to the Slides format labels Nov 8, 2017
@mpacer
Copy link
Member

mpacer commented Nov 14, 2017

@damianavila do you have any thoughts on how to best address this?

@fperez
Copy link
Member Author

fperez commented Nov 17, 2017

As an addendum: why don't we just use the CDN prefix by default? We already hardcode CDN paths for require/jquery in the conversion, why not also for reveal? Doing that would have the benefit of at least working out of the box for pushing slides to any simple hosting environment (e.g. github pages). Right now, users will waste time finding the special --reveal-prefix incantation that works.

Then we can tackle the 100% offline issue as a separate one.

@damianavila
Copy link
Member

But if the same exact HTML file is then served locally

As far as I know this should work (it worked OK in the past), quick question how are you servind that directory? With --post serve? Or something else?

But as mentioned in #296, we want to make it possible to easily serve slides fully offline too, so this workaround (while useful for some cases) isn't a full solution.

I agree it is not a solution for the people looking to use it locally

Improve the instructions to clarify exactly what to do for a locally hosted reveal.js, that actually works (I tried multiple flavors of --reveal-prefix with ., ./, ./reveal.js, and none worked).

Let me check this, as I said before, it was working some time ago.

Also provide the explicit tip of using the CDN path (--reveal-prefix "https://cdnjs.cloudflare.com/ajax/libs/reveal.js/3.1.0"), which is confirmed to work and for some cases works. I can make a PR for this if it's confirmed it's the right thing to do.

This covers the use case when you want to point to another resource to hit the reveal.js content.
But it is not a solution to make it work with the local library, so we need to first check why is not working with the local library and then add this piece of info behind the proper use case.

why don't we just use the CDN prefix by default?

Well, --post serve is the place where we redirect to the CDN or use the local library... so, if you want to serve your slides, you only need to add the reveal.js library alongside with the slides.html file and you are done, but if that is not working...

OK, give some time to check this behavior and see what I can do to make the experience better.

@damianavila
Copy link
Member

@fperez, with nbconvert 5.3.1, I can see the slides without any problems if I do not use --post serve and I put reveal.js library alongside with the generated slides.html file. I do not even need to start a http.server (from the file browser I can double click the slides.html file and see the slideshow).

Which version of reveal.js are you using? In my case I downloaded the 3.5.0 version (the same one that we are currently pointing out when we use --post serve).

We already hardcode CDN paths for require/jquery in the conversion, why not also for reveal?

Essentially because you need a local reveal.js to use the speaker notes... and a lot of people use the speaker notes. The idea was to make easy for people to have the complete reveal.js experience and provide an option (reveal-prefix), for instance, in case you want a "reduced" experience but easier to push to Github. Maybe we can invert the situation, but it would be probably disruptive for all the people used to the current workflow.

Right now, users will waste time finding the special --reveal-prefix incantation that works.

Btw, as a general thought, I think we (I) need to do a better job documenting the options we have so people does not have to guess them. Let me try to fix this missing info in the coming weeks 😉

@mpacer
Copy link
Member

mpacer commented Jan 10, 2018

I'm going to make a PR for changing the default… given that the local way of doing this doesn't work right now. It's a dirty fix, but it'll work.

@takluyver
Copy link
Member

I think the issue here was the version of reveal.js in use. I can reproduce the error that @fperez saw by putting reveal 2.6.2 as the adjacent reveal.js/ folder, but it works as expected when I use reveal 3.0.0 or 3.6.0 (the latest release).

So I'd suggest that we more clearly document that it expects reveal.js 3.x instead of changing the code.

@takluyver
Copy link
Member

For reference, this has probably been the case since PR #65, over two years ago.

takluyver added a commit to takluyver/nbconvert that referenced this issue Jan 15, 2018
@takluyver
Copy link
Member

See #735.

@damianavila
Copy link
Member

So I'd suggest that we more clearly document that it expects reveal.js 3.x instead of changing the code.

+1 , I did not try with version 2.x because it is so old... thanks for caching that difference.

mpacer added a commit to mpacer/nbconvert that referenced this issue Jan 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs format:Slides pertains to exporting to the Slides format
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants