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

Merged
merged 8 commits into from Feb 1, 2018

Conversation

Projects
None yet
3 participants
@damianavila
Copy link
Member

damianavila commented Jan 26, 2018

Some more changes after PR #732.

@damianavila damianavila requested review from mpacer and takluyver Jan 26, 2018

@@ -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

This comment has been minimized.

@mpacer

mpacer Jan 26, 2018

Member

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.

This comment has been minimized.

@mpacer

mpacer Jan 26, 2018

Member

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.

This comment has been minimized.

@damianavila

damianavila Jan 26, 2018

Author Member

Fair enough, feel free to push the removal 😉


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

This comment has been minimized.

@mpacer

mpacer Jan 26, 2018

Member

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.

This comment has been minimized.

@damianavila

damianavila Jan 26, 2018

Author Member

That's OK for me.

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Jan 26, 2018

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.

@damianavila

This comment has been minimized.

Copy link
Member Author

damianavila commented Jan 26, 2018

I’ll push some changes in a bit that will bring this closer to what I imagine.

Thank you for doing this!

So… that probably means I’m being more strict on the narrative structure that I normally would.

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...

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Jan 29, 2018

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).

mpacer added some commits Jan 29, 2018

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

This comment has been minimized.

@takluyver

takluyver Jan 30, 2018

Member

does not ?

This comment has been minimized.

@damianavila

damianavila Jan 30, 2018

Author Member

I think so...

This comment has been minimized.

@mpacer

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.

This comment has been minimized.

@takluyver

takluyver Jan 30, 2018

Member

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.

This comment has been minimized.

@damianavila

damianavila Jan 30, 2018

Author Member

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.

This comment has been minimized.

@mpacer

mpacer Jan 30, 2018

Member

doesn't the simple http.server only serve http and not https? isn't it required to be an https server?

This comment has been minimized.

@mpacer

mpacer Jan 30, 2018

Member

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.

This comment has been minimized.

@damianavila

damianavila Jan 30, 2018

Author Member

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.

This comment has been minimized.

@mpacer

mpacer Jan 31, 2018

Member

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?

@mpacer

This comment has been minimized.

Copy link
Member

mpacer commented Jan 31, 2018

@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 --post serve. If you want me to remove it, I will but I also think that we could hash that out in a later PR.

@takluyver

This comment has been minimized.

Copy link
Member

takluyver commented Feb 1, 2018

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."

@takluyver takluyver merged commit ef3218c into jupyter:master Feb 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@damianavila damianavila deleted the damianavila:fix_slide_docs branch Feb 1, 2018

@mpacer mpacer added this to the 5.4 milestone Feb 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment