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

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

Merged
merged 8 commits into from
Feb 1, 2018

Conversation

damianavila
Copy link
Member

Some more changes after PR #732.

@@ -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
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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 😉


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
Copy link
Member

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.

Copy link
Member Author

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.

@mpacer
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
Copy link
Member Author

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

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so...

Copy link
Member

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.
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

@mpacer mpacer Jan 31, 2018

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?

@mpacer
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
Copy link
Member

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
@damianavila damianavila deleted the fix_slide_docs branch February 1, 2018 16:18
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants