change default for slides to direct to the reveal cdn rather than locally #732

mpacer · 2018-01-10T20:59:57Z

This is the quick fix suggested by @fperez (to change the default) for #702.

My intuition is that we should provide a way to to create slides in a "stand-alone" mode. That mode will actually solve the actual problem with local copy of reveal that we're running into in #702.
That would probably also download the relevant copy of the reveal library into the output directory and maybe a bunch more.

fperez · 2018-01-12T19:50:39Z

This looks good to me, but a 2nd pair of eyes would be useful, as I've been away from the code for too long to trust that I won't miss something...

fperez · 2018-01-12T19:51:22Z

Thx a lot, btw! Having this work out of the box (even if it doesn't support 100% offline use) will help a lot of regular users.

mpacer · 2018-01-13T22:37:25Z

@takluyver @damianavila could either of you give this a look before we merge it? I'd like to get out a release soon and I'd like this to be part of it.

takluyver · 2018-01-15T15:27:59Z

I'm wary of slapping a band-aid fix on for a problem that, based on the discussion on #702, we don't seem to properly understand. Damian suggested that this change risks breaking some other people's workflow, and I'd rather leave the problem where it is for another release - where people have likely found some kind of workaround - rather than creating a new problem for a different set of users and causing more confusion.

(The users experiencing the problem at the moment will likely protest that theirs is the typical use case, and we should just fix their bug, because the things it breaks are bound to be less important. We're all prone to assuming that we're typical. I'm not trying to make a judgment on whose use case is more important, just that a consistent problem is better than switching between problems.)

Clearly that answer isn't very satisfactory, so I'm going to try to investigate what's going on with slides.

damianavila · 2018-01-15T16:57:23Z

Sorry perople for being late here, I was sick the last few days.

In general agree with @takluyver here. I am more than open to discuss the default change, but if we take that path, we need to make it in a more general PR changing not only the default value but also other parts of the slides architecture. Changing just this value I think will bring us more problems than solutions. This is why +1 @takluyver PR on docs and I would be -1 into merging this one as is.

mpacer · 2018-01-18T03:56:32Z

I’ll merge the docs… but I do think this is a default that benefits users “in-the-know” better than users who don’t get these details. It is not just that this is a typical use case but is the major reason people have quoted as their problems with using the slides export. Having a local copy of reveal is much further from “just working” than relying on the cdn.

We should definitely provide a way to handle the local filesystem use case and I’m ok including that in this PR if that’s what is needed to address this default issue… but I genuinely think it should be changed to support easier new users and to support webpages for slides that work by default when opened without needing to pull from a relative directory (which is often undesirable, e.g., in the case of github.io pages).

damianavila · 2018-01-19T12:07:52Z

If we change the current default, we need to tell people who use the speaker notes (which I presume is a big percentage) to change their workflow from:

jupyter nbconvert my_slide.ipynb --to slides --post serve

to

jupyter nbconvert my_slide.ipynb -- to slides --reveal-prefix="reveal.js" --post serve

Otherwise their speaker notes will be busted. Do you think that is acceptable for the users? It is pretty difficult to me to realize if this change is annoying enough to make people angry... it is not annoying for me but I am sure I am biased in my response to this change.
@takluyver @mpacer @fperez thoughts?

takluyver · 2018-01-19T12:27:58Z

My thoughts (as above) are that I don't want to try to judge which course creates the bigger inconvenience for the larger group. Rather, I'd apply the status quo argument: I'd prefer to let people live with the current inconvenience for another release than change it and create a different inconvenience. But as the main nbconvert maintainer, @mpacer has the final call on whether this goes in or not.

Longer term, I think we all agree that we need to look more closely at how our slides export deals with external resources. We certainly want a way to produce a single file, and a way to have slides that work completely offline, not pulling anything from a CDN. I'd also like to end the dependence on the serve post-processor (and get rid of the notion of post-processors in nbconvert altogether).

What are the limitations on the speaker notes feature? Do I recall that it only works when served over HTTP (i.e. not from a file:// URL)? Do you know why that is the case?

damianavila · 2018-01-19T13:04:11Z

What are the limitations on the speaker notes feature? Do I recall that it only works when served over HTTP (i.e. not from a file:// URL)? Do you know why that is the case?

The CDN is not serving the notes.html file: cdnjs/cdnjs#3317
I guess we can serve that specific file (alonside with notes.js) from another place?

takluyver · 2018-01-19T13:07:01Z

Do you want to open a separate issue about the speaker notes thing so this discussion doesn't get sidetracked (more than I already sidetracked it ;-)?

damianavila · 2018-01-19T13:27:38Z

Do you want to open a separate issue about the speaker notes thing so this discussion doesn't get sidetracked (more than I already sidetracked it ;-)?

Good idea!

takluyver · 2018-01-19T14:38:03Z

Actually, the more I think about this, the more I agree with M - I think it would be nicer to have slides export produce a file you can use without a special server, even if that breaks speaker notes.

But if the speaker notes just needs the CDN to serve HTML, could we use rawgit as the prefix? I.e. this URL works:

https://cdn.rawgit.com/hakimel/reveal.js/3.5.0/plugin/notes/notes.html

damianavila · 2018-01-19T14:59:12Z

Actually, the more I think about this, the more I agree with M - I think it would be nicer to have slides export produce a file you can use without a special server, even if that breaks speaker notes.

Yep, this is starting to be my feeling as well... despite the change in the status quo.

But if the speaker notes just needs the CDN to serve HTML, could we use rawgit as the prefix? I.e. this URL works:

I quickly tried on the fly but there is still some other pieces needed, I guess, because I get errors about Reveal.js not defined and Mixed content...

takluyver · 2018-01-19T15:12:54Z

Would this work as a way to deal with the offline use case:

When nbconvert produces the file, let all the URLs point to CDNs, so it's a small file that works standalone. Then have a separate tool that can read the HTML file, fetch the necessary JS and CSS, and either place them alongside the file, or stuff them into <script> and <style> tags inside it to produce an offline version. I've just had some fun writing a very simple version of this tool, but it feels like something that lots of other people must have written already.

takluyver · 2018-01-19T15:28:47Z

I quickly tried on the fly but there is still some other pieces needed, I guess, because I get errors about Reveal.js not defined and Mixed content...

I see those too. The problem is that the speaker notes page is loaded over HTTPS, but then it tries to connect to the local server showing the presentation, which is HTTP. I can work around it for now by changing the rawgit CDN address to http://..., but that will stop working next month.

takluyver · 2018-01-19T15:29:55Z

You can also work around it in Firefox by temporarily disabling the mixed content protection - click on the padlock icon and find the button - but I don't know if I'd rely on that working either.

damianavila · 2018-01-19T15:49:35Z

Would this work as a way to deal with the offline use case:

Interesting idea. I think it should work.

I see those too. The problem is that the speaker notes page is loaded over HTTPS, but then it tries to connect to the local server showing the presentation, which is HTTP. I can work around it for now by changing the rawgit CDN address to http://..., but that will stop working next month.

As a quick turnaround we can serve these files, notes.js and notes.html, from the jupyter cdn, right?
We are already serving css from there in https and http: http://cdn.jupyter.org/notebook/5.1.0/style/style.min.css

takluyver · 2018-01-19T16:11:22Z

I think the speaker notes should work from localhost if we can get revealjs from any domain where we can rely on it being plain HTTP for some time yet. CC @minrk @rgbkrk

rgbkrk · 2018-01-19T19:45:38Z

nbconvert/exporters/slides.py

@@ -90,7 +90,7 @@ def _reveal_url_prefix_default(self):
            warn("Please update RevealHelpPreprocessor.url_prefix to "
                 "SlidesExporter.reveal_url_prefix in config files.")
            return self.config.RevealHelpPreprocessor.url_prefix
-        return 'reveal.js'
+        return 'https://cdnjs.cloudflare.com/ajax/libs/reveal.js/3.1.0'


Switch this to:

return '//cdnjs.cloudflare.com/ajax/libs/reveal.js/3.1.0'

to provide a URL that the browser will adhere to HTTPS or HTTP for the user's page.

I'm not sure if it will work, because the CDN redirects HTTP requests to HTTPS. Worth a try, though - thanks @rgbkrk

I used:

{ src: "//cdn.rawgit.com/hakimel/reveal.js/3.5.0/plugin/notes/notes.js",

And it seems to work for me... @takluyver can you check that as well?

Nope, it still opens the speaker notes view with an https URL, unfortunately.

Additionally, using // breaks viewing the slides from a file:// URL (i.e. opening the file without running a local http server), whereas that works if we specify the protocol.

@damianavila the rawgit URL works for now, but I think it will stop working next month, when rawgit starts redirecting all HTTP requests to HTTPS.

I tested with the URL @rgbkrk suggested - the notes.html file for speaker view returns a 403, but you can still see that it's opening with an https URL, which is the cause of the 'mixed content' problem.

Right, I forgot - that was what I was asking @rgbkrk and @minrk about: would it be OK to host a copy of reveal.js on cdn.jupyter.org so that we can access it with plain HTTP?

I'd prefer to pare down the use of cdn.jupyter.org, especially for things that aren't our own in favor of things like unpkg. The only reason for cdn.jupyter I can see is the non-npm packages we own (basically just the notebook-classic js/css). But pointing to unpkg or another CDN for reveal.js ought to work.

BTW, isn't it fine to load https resources on an http page, just not the other way around? Plus, https will work for a file:// page, whereas // will not, so I think explicit https is preferable to //, unless I'm missing something.

The https:// URL works except for the speaker notes. Speaker notes open an HTML file at a URL based on the one reveal is loaded from (e.g. https://cdn.rawgit.com/hakimel/reveal.js/3.5.0/plugin/notes/notes.html ). That page connects back to the server your slides are served from.

So if the notes page is an HTTP URL, it works. But if the notes page is an HTTPS URL and you're serving the slides with plain HTTP, the browser doesn't like the insecure connection.

But it's a good point that it would also be a problem the other way around - we can't load reveal with HTTP if you serve the slides using HTTPS.

So I think we just have to use HTTPS, and say that you have to use a local copy of reveal.js if you want to use speaker notes.

So I think we just have to use HTTPS, and say that you have to use a local copy of reveal.js if you want to use speaker notes.

👍

So I think we just have to use HTTPS, and say that you have to use a local copy of reveal.js if you want to use speaker notes.

I am OK with that as well after the whole discussion.

damianavila · 2018-01-20T11:42:03Z

nbconvert/exporters/slides.py

@@ -90,7 +90,7 @@ def _reveal_url_prefix_default(self):
            warn("Please update RevealHelpPreprocessor.url_prefix to "
                 "SlidesExporter.reveal_url_prefix in config files.")
            return self.config.RevealHelpPreprocessor.url_prefix
-        return 'reveal.js'


If we finally accept this change, @mpacer you need to point to reveal 3.5.0 which is the version we already have in the serve postprocessor.

takluyver · 2018-01-22T20:16:04Z

OK, thanks everyone. @mpacer I think the remaining thing to do is to update the URL to 3.5.0, which is the version we refer to elsewhere for reveal.js.

damianavila · 2018-01-22T21:29:46Z

I think the remaining thing to do is to update the URL to 3.5.0, which is the version we refer to elsewhere for reveal.js.

I would say we should update the docs PR #735 to add the following incantation for those who want to use the speaker notes:

jupyter nbconvert my_slide.ipynb -- to slides --reveal-prefix="reveal.js" --post serve

takluyver · 2018-01-23T13:38:38Z

Good point, let's not forget to update the docs!

mpacer · 2018-01-23T17:00:18Z

Ok I’ll make the change to update the version number later today.

mpacer · 2018-01-23T17:00:34Z

Thanks everyone for hashing this out!

mpacer · 2018-01-23T18:07:13Z

Do you want me to merge the docs PR into this one? so that it's all changed at once?

damianavila · 2018-01-23T19:20:59Z

Do you want me to merge the docs PR into this one? so that it's all changed at once?

I think it is good idea, but it is @takluyver 's PR so let's wait for his feedback, I think.

takluyver · 2018-01-23T19:40:08Z

Yup, it's probably a good idea to merge #735 in here, because then the docs need another change as part of this PR, since putting revealjs beside the HTML file is no longer sufficient.

…uick fix for jupyter#702

Closes jupytergh-702

emphasize that the key feature for speaker notes is a local copy. Give an example of how to set up speaker notes.

rgbkrk · 2018-01-26T02:10:54Z

docs/source/usage.rst

+of reveal.js and then that you redirect the script away from your CDN to your 
+local copy.
+
+To make this clearer, let's look at an example. 


rgbkrk · 2018-01-26T02:11:16Z

docs/source/usage.rst

+appending the command line flag ``--post serve`` to the above command. This
+will not allow you to use speaker notes if you do not have a local copy of
+reveal.js. 
+


Solid prose here

rgbkrk · 2018-01-26T02:11:31Z

nbconvert/exporters/slides.py

@@ -90,13 +95,14 @@ def _reveal_url_prefix_default(self):
            warn("Please update RevealHelpPreprocessor.url_prefix to "
                 "SlidesExporter.reveal_url_prefix in config files.")
            return self.config.RevealHelpPreprocessor.url_prefix
-        return 'reveal.js'
+        return 'https://cdnjs.cloudflare.com/ajax/libs/reveal.js/3.5.0'


takluyver · 2018-01-26T10:20:12Z

Alright, time to press the button on this. Thanks everyone for the discussion - it's a small technical change, but I'm pleased we've taken the time to think through it and make sure the docs are clear. :-)

damianavila · 2018-01-26T11:03:10Z

docs/source/usage.rst

+
+This will create file ``your_talk.slides.html``, which you should be able to 
+access with ``open your_talk.slides.html``. To access the speaker notes, press 
+``s`` after the slides load and they should open in a new window.


Should we mention that the timer inside the speaker notes will only work if you are serving the slides?

Ok I wasn’t sure why that was I thought I had a weird bug and that the serving was required to coordinate 2 windows… why does the timer only work if you serve the slides but it still allows you to communicate between 2 windows?

why does the timer only work if you serve the slides but it still allows you to communicate between 2 windows?

Good question! I have not researched the cause yet. I just verified the behavior.

damianavila · 2018-01-26T11:03:12Z

docs/source/usage.rst

+access with ``open your_talk.slides.html``. To access the speaker notes, press 
+``s`` after the slides load and they should open in a new window.
+
+This should also allow you to use your slides without an internet connection.


Well, this is not true... we are loading other things from the internet, such as font-awesome, jquery, require, mathjax... and reveal.js itself load google fonts (although they have a fallback in case of lack of connectivity.
This statement as is can cause confussion, I would prefer to delete it.

I was making that claim based on the previous docs:

To make slides that don't require an internet connection, just place the Reveal.js library in the same directory where your_talk.slides.html is.

So at least this isn’t a regression…

Makes me think we should have a way to package something that we can guarantee works in an offline context (e.g., where we download the relevant files to the directory in question since this is a more people would like to use).

Makes me think we should have a way to package something that we can guarantee works in an offline context (e.g., where we download the relevant files to the directory in question since this is a more people would like to use).

Yep, I was thinking the same, maybe some nbconvert script to download all the files needed?

damianavila · 2018-01-26T11:04:36Z

@takluyver @mpacer sorry for the post-merge comments, but I think they are enough important to change some things and prevent users confusion.

takluyver · 2018-01-26T11:08:59Z

@damianavila no problem, there's always room for a bit more review. Do you want to make a PR yourself?

damianavila · 2018-01-26T11:27:46Z

@damianavila no problem, there's always room for a bit more review. Do you want to make a PR yourself?

Sure, will submit in a few minutes.

mpacer changed the title ~~change default for slides to direct to the cdn rather than locally~~ change default for slides to direct to the reveal cdn rather than locally Jan 10, 2018

mpacer mentioned this pull request Jan 18, 2018

Clarify that reveal.js 3.x is needed #735

Closed

rgbkrk reviewed Jan 19, 2018

View reviewed changes

damianavila reviewed Jan 20, 2018

View reviewed changes

mpacer and others added 5 commits January 23, 2018 16:10

change default for slides to direct to the cdn rather than locally, q…

2b74ad8

…uick fix for jupyter#702

Clarify that reveal.js 3.x is needed

ff3ef1e

Closes jupytergh-702

Instructions for fetching reveal.js 3.6.0

6fcd4a6

Use reveal 3.5 for consistency

b83a599

update version of reveal to 3.5

2f508ca

mpacer force-pushed the better_reveal_prefix branch from b2b5493 to 2f508ca Compare January 24, 2018 00:10

mpacer added 2 commits January 25, 2018 12:47

modify help string to point at the usage docs.

880a388

Improve usage docs for the reveal slideshow

a607957

emphasize that the key feature for speaker notes is a local copy. Give an example of how to set up speaker notes.

rgbkrk approved these changes Jan 26, 2018

View reviewed changes

takluyver merged commit 5f5ae49 into jupyter:master Jan 26, 2018

damianavila reviewed Jan 26, 2018

View reviewed changes

damianavila mentioned this pull request Jan 26, 2018

Remove offline statement and add some clarifications in slides docs #743

Merged

takluyver mentioned this pull request Feb 5, 2018

HTML/slides export without CDNs #754

Open

mpacer added this to the 5.4 milestone Feb 8, 2018

blink1073 mentioned this pull request Feb 28, 2018

Download as reveal.js slides results in a single page jupyterlab/jupyterlab#4067

Closed

change default for slides to direct to the reveal cdn rather than locally #732

change default for slides to direct to the reveal cdn rather than locally #732

Conversation

mpacer commented Jan 10, 2018

fperez commented Jan 12, 2018

fperez commented Jan 12, 2018

mpacer commented Jan 13, 2018

takluyver commented Jan 15, 2018

damianavila commented Jan 15, 2018

mpacer commented Jan 18, 2018

damianavila commented Jan 19, 2018

takluyver commented Jan 19, 2018

damianavila commented Jan 19, 2018

takluyver commented Jan 19, 2018

damianavila commented Jan 19, 2018

takluyver commented Jan 19, 2018

damianavila commented Jan 19, 2018

takluyver commented Jan 19, 2018

takluyver commented Jan 19, 2018

takluyver commented Jan 19, 2018

damianavila commented Jan 19, 2018

takluyver commented Jan 19, 2018

rgbkrk Jan 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takluyver commented Jan 22, 2018

damianavila commented Jan 22, 2018 • edited Loading

takluyver commented Jan 23, 2018

mpacer commented Jan 23, 2018

mpacer commented Jan 23, 2018

mpacer commented Jan 23, 2018

damianavila commented Jan 23, 2018

takluyver commented Jan 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

takluyver commented Jan 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damianavila commented Jan 26, 2018

takluyver commented Jan 26, 2018

damianavila commented Jan 26, 2018

rgbkrk Jan 19, 2018 •

edited

Loading

damianavila commented Jan 22, 2018 •

edited

Loading