-
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
Remove offline statement and add some clarifications in slides docs #743
Conversation
docs/source/usage.rst
Outdated
@@ -133,6 +133,10 @@ following commands inside the directory: | |||
git checkout 3.5.0 | |||
cd .. | |||
|
|||
Alternative, you can download a zip (or tar.gz) file containing reveal.js from |
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.
Honestly I’d prefer to keep this simpler in this location. If we really want to cover a lot of options on how to do this I think we should have a separate page. But otherwise I’d like this to be a linear flow.
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.
To explain why: the people who benefit most from this are going to be those who don’t quite understand why they need to do this. They’ll have the easiest time copying and pasting some commands, even if they don’t fully understand those commands. Telling someone to find a resource that they may or may not understand and then to conditionally manipulate it (tar -xfd
or unzip
) using tools they may or may not already know introduces unnecessary complication and uncertainty. We should aim to be as simple as possible, which ideally implies a workflow uncluttered with as few decisions as possible.
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.
Fair enough, feel free to push the removal 😉
docs/source/usage.rst
Outdated
|
||
This should also allow you to use your slides without an internet connection. | ||
``s`` after the slides load and they should open in a new window. Keep in mind | ||
that if you want a functional timer inside the speaker notes, you need to serve |
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.
This is somewhat complicated syntax. I also think I’d prefer a caveat with no explanation of how to fix it to come earlier in the paragraph. The explanation would be given by reference with a hyperlink to a new section header that lives above the statement about the https server which should explicitly mention the bit about the timer.
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.
That's OK for me.
I’ll push some changes in a bit that will bring this closer to what I imagine. It’s really important to me that we write these docs in a way that novices can use this utility without getting needlessly frustrated. I’ve seen it a lot and I want to avoid it going forward. So… that probably means I’m being more strict on the narrative structure that I normally would. |
Thank you for doing this!
I can understand this need and I am more than open to the changes you proposed. As you know, English is not my mother language so I can be a little bit cryptic sometimes... |
I thought your prose was fine, just better fit for a different purpose (e.g., a page that described in greater detail how to use the notebook for slides — which I think may make sense to include inside the nbconvert docs). |
I found that if the config_options.rst file was present, the make html was not recreating them, leading to out of date configuration docs.
docs/source/usage.rst
Outdated
the slides (see next paragraph for details). | ||
``s`` after the slides load and they should open in a new window. | ||
|
||
Note: a local copy of reveal.js does allow slides that will run completely |
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.
does not ?
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.
I think so...
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.
oops
|
||
Fortunately, ``nbconvert`` makes this fairly straightforward through the use of | ||
the ``ServePostProcessor``. To activate this server, we append the command line | ||
flag ``--post serve`` to our call to nbconvert. |
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.
Once you've got reveal.js set up next to the HTML file, I don't think you need the post processor - we could recommend python3 -m http.server
to start a server.
If we can recommend this, it also gets us one step closer to getting rid of the postprocessor entirely.
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.
As you said, you don't need the post-processor, but it makes things easier for you.
I am +1 about the removal of post-processors, but I would keep the --post serve
concept (maybe just calling it serve
and implemented w/o post-processors) instead of recommending the python3 -m http.server
.
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.
doesn't the simple http.server only serve http and not https? isn't it required to be an https server?
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.
Ok… well I just tested it and there is one tiny issue with python3 -m http.server
, which is that it doesn't auto-open the file in the browser… meaning the person needs to navigate to localhost:8000/your_talk.slides.html
in their browser which is both a context switch (cli to browser) and introduces a new layer of complexity (correctly arriving at the slides page relative to an implicit port statement).
We can add the port declaration to the command python3 -m http.server 8000
even if we keep it the default, but I still think that this is an additional opportunity for users to become confused.
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.
meaning the person needs to navigate to localhost:8000/your_talk.slides.html in their browser which is both a context switch (cli to browser) and introduces a new layer of complexity (correctly arriving at the slides page relative to an implicit port statement).
I agree, this is why I would keep the --post serve
until we provide a --serve
option that is not based in a postprocessor.
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.
So… for this docs PR… I think we should still use --post serve
@takluyver are you ok with that or do you feel strongly that we should include the slightly more verbose python3 -m http.server
style instructions in order to clear the path for the removal of the postprocessors?
@takluyver I think you’re the only one who should merge this at this point, as both @damianavila and I have contributed to the PR and we’re on the same page regarding keeping the bit about |
Sorry, my reply seems to have got lost somewhere. I tried a couple of days ago to say something like "I don't feel strongly, go for it." |
Some more changes after PR #732.