Update reveal, fix scrolling and add transition option #600

damianavila · 2017-05-31T20:51:08Z

Better per-slide scrolling (defaulting to False but with options to enable it with
--SlidesExporter.reveal_scroll=True)
Upgrade to latest reveal.js (3.5.0)
Add option to pass the transition we want.

…nable it), upgrade to latest reveal.js and add one option to pass the transition we want.

takluyver · 2017-06-01T10:52:23Z

nbconvert/exporters/slides.py

+        Name of the reveal.js transition to use.
+
+        The list of themes that ship by default with reveal.js are:
+        default, cube, page, concave, zoom, none.


Should this say 'list of transitions'? And shouldn't linear be in the list, as it's set as the default?

Yep... copy pasted stuff and forgot to fix it before pushing.

Fixed and changed the default value to slide (new name for linear transition).

takluyver · 2017-06-01T10:53:40Z

nbconvert/exporters/slides.py

+
+    reveal_scroll = Bool(False,
+        help="""
+        Whether to enable/disable scrolling per slide basis


Probably clearest to say something like:

If True, enable scrolling within each slide

If this is disabled, does scrolling move between slides? Or just do nothing?

If True, enable scrolling within each slide

I agree.

If this is disabled, does scrolling move between slides? Or just do nothing?

Just do nothing... scrolling to move between slides can be configured with other reveal.js parameter (mousewheel IIRC), but we don't surface that in our template.

takluyver · 2017-06-01T10:56:12Z

nbconvert/templates/html/slides_reveal.tpl

+                .filter(function() {
+                  return $(this).height() > h;
+                })
+                .css('height', hpx)


Does it work to use a height like 95%, or even CSS calc (calc(100% - 20px)), rather than doing calculations in JS?

I did not try to use CSS calc... will take a look in your suggestions.

Does it work to use a height like 95%

Nop, it cut the stuff at the very top.

even CSS calc

Neither... in both cases I get the cut off

It seems I need to specify the the absolute height in pixel to make it work.

Gah, CSS. It's probably doing the relative height relative to the wrong parent.

One more thing to try: calc(100vh - 20px). vh units are 1% of the height of the viewport, so 100vh should be the full height available in the window.

Great, vh tested and worked. Pushed a commit fixing it. Thanks for letting learn about this CSS calc stuff!

This looks along the lines of the kinds of things to get around 'auto' heights in the case of animations. There should be a way to work natively in pixels rather than treating it as a string manipulation problem though. I forget what that is off the top of my head though.

I just pushed a commit using @takluyver suggested approach which get rid of the awkward string manipulation.

takluyver · 2017-06-01T10:58:40Z

nbconvert/templates/html/slides_reveal.tpl

+
+        Reveal.addEventListener( 'slidechanged', function( event ) {
+          // check and set the scrolling slide every time the slide change
+          var scroll = "{{resources.reveal.scroll}}";


Making the boolean into a string is a bit ugly. We could either:

Have the condition in the template language, {% if resources.reveal.scroll %}

Convert it to a JS boolean {{ resources.reveal.scroll | json_dumps }}

bit ugly

Very ugly... Need to be fixed... I like the if approach... I will try that.

I change my mind and I went with the json_dumps approach (and simplify the structure a little bit).

…ault (previous know as linear but that name was deprecated).

… bit

… progress and the scrollbar as well

damianavila · 2017-06-01T23:34:07Z

Push some other minor changes to have a nicer scrollbar and a shared color between several pieces such as controls, progressbar and scrollbar.

damianavila · 2017-06-01T23:49:35Z

Btw, I cancelled the first job because I push to origin by mistake (instead of remote branch).

takluyver · 2017-06-02T13:02:22Z

nbconvert/templates/html/slides_reveal.tpl

+{
+  width: 6px;
+  height: 6px;
+}


I didn't know CSS can override the scrollbar. Are you sure you want to do that? It seems like the kind of thing that people might get angry about.

I don't have strong intuitions around this, what would the downsides be?

It is overriding stuff on the slideshow... and this scroll is there specifically to scroll the slide.
Btw, this gives me the opportunity to make the scroll closer (in look and feel) to other elements... otherwise you get the "system" view which it is not fitting at all the the slide feeling.

mpacer

Overall this seems solid. I'm not the most familiar with reveal and how to make it play nicely with css.

What I don't understand is why we need to do this kind of custom scrolling in the first place. What is it about our stuff is breaking the standard scroll?

mpacer · 2017-06-02T16:55:53Z

nbconvert/templates/html/slides_reveal.tpl

+                .filter(function() {
+                  return $(this).height() > h;
+                })
+                .css('height', hpx)


This looks along the lines of the kinds of things to get around 'auto' heights in the case of animations. There should be a way to work natively in pixels rather than treating it as a string manipulation problem though. I forget what that is off the top of my head though.

damianavila · 2017-06-02T17:05:04Z

What I don't understand is why we need to do this kind of custom scrolling in the first place. What is it about our stuff is breaking the standard scroll?

People using nbconverted slides wanted from the very beginning to have scrolling slides. And we provide that even when reveal.js itself discouraged any use of scrolling inside the slides. As a consequence, in repetitive cases, the scrolling stuff broke. In this PR I am trying to make the scrolling stuff just a configurable thing and if the user really wanted, they can have it.

You have here more stuff on the scrolling discussion if you are interested: #78

mpacer · 2017-06-05T20:49:33Z

Ok I'll merge.

damianavila · 2017-06-07T13:15:51Z

Thanks @mpacer and @takluyver for you helpful review.

Better per-slide scrolling (defaulting to False but with options to e…

d5c50fe

…nable it), upgrade to latest reveal.js and add one option to pass the transition we want.

This was referenced May 31, 2017

Review the scroll functionality in the slideshow #78

Closed

Slides failing to render correctly #157

Closed

takluyver reviewed Jun 1, 2017

View reviewed changes

damianavila added 4 commits June 1, 2017 16:24

Correct docstrings about the transitions and use the slide one as def…

16c843f

…ault (previous know as linear but that name was deprecated).

Correct docstring for scroll option.

acbd41f

Get the boolean in the correct way and simplify the function a little…

8eeeb2f

… bit

Better (without track) scrolling bar and unified colors for controls,…

51d5c77

… progress and the scrollbar as well

damianavila requested review from takluyver and mpacer June 1, 2017 23:50

takluyver reviewed Jun 2, 2017

View reviewed changes

Use CSS calc instead of awful px based height

2364f4c

mpacer reviewed Jun 2, 2017

View reviewed changes

mpacer merged commit 760cd49 into jupyter:master Jun 5, 2017

mpacer added this to the 5.3 milestone Jun 5, 2017

damianavila deleted the upgrade_reveal branch June 7, 2017 13:14

This was referenced Jun 7, 2017

Can't turn off scroll bar on the slides #152

Closed

cannot use reveal.js print-pdf option #596

Closed

mpacer added unlogged and removed unlogged labels Aug 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update reveal, fix scrolling and add transition option #600

Update reveal, fix scrolling and add transition option #600

damianavila commented May 31, 2017 •

edited

Loading

takluyver Jun 1, 2017

damianavila Jun 1, 2017

damianavila Jun 1, 2017

takluyver Jun 1, 2017

damianavila Jun 1, 2017

damianavila Jun 1, 2017

takluyver Jun 1, 2017

damianavila Jun 1, 2017

damianavila Jun 1, 2017

takluyver Jun 2, 2017

damianavila Jun 2, 2017

mpacer Jun 2, 2017

damianavila Jun 2, 2017

takluyver Jun 1, 2017

damianavila Jun 1, 2017

damianavila Jun 1, 2017

damianavila commented Jun 1, 2017

damianavila commented Jun 1, 2017

takluyver Jun 2, 2017

mpacer Jun 2, 2017

damianavila Jun 2, 2017 •

edited

Loading

mpacer left a comment

mpacer Jun 2, 2017

damianavila commented Jun 2, 2017 •

edited

Loading

mpacer commented Jun 5, 2017

damianavila commented Jun 7, 2017

Update reveal, fix scrolling and add transition option #600

Update reveal, fix scrolling and add transition option #600

Conversation

damianavila commented May 31, 2017 • 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

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 Jun 1, 2017

damianavila commented Jun 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damianavila Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

mpacer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damianavila commented Jun 2, 2017 • edited Loading

mpacer commented Jun 5, 2017

damianavila commented Jun 7, 2017

damianavila commented May 31, 2017 •

edited

Loading

damianavila Jun 2, 2017 •

edited

Loading

damianavila commented Jun 2, 2017 •

edited

Loading