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

Update reveal, fix scrolling and add transition option #600

Merged
merged 6 commits into from Jun 5, 2017

Conversation

Projects
None yet
3 participants
@damianavila
Copy link
Member

damianavila commented May 31, 2017

  • 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.
Better per-slide scrolling (defaulting to False but with options to e…
…nable it), upgrade to latest reveal.js and add one option to pass the transition we want.
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.

This comment has been minimized.

@takluyver

takluyver Jun 1, 2017

Member

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

This comment has been minimized.

@damianavila

damianavila Jun 1, 2017

Author Member

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

This comment has been minimized.

@damianavila

damianavila Jun 1, 2017

Author Member

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


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

This comment has been minimized.

@takluyver

takluyver Jun 1, 2017

Member

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?

This comment has been minimized.

@damianavila

damianavila Jun 1, 2017

Author Member

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.

This comment has been minimized.

@damianavila

damianavila Jun 1, 2017

Author Member

Fixed.

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

This comment has been minimized.

@takluyver

takluyver Jun 1, 2017

Member

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

This comment has been minimized.

@damianavila

damianavila Jun 1, 2017

Author Member

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

This comment has been minimized.

@damianavila

damianavila Jun 1, 2017

Author Member

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

slide2

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

This comment has been minimized.

@takluyver

takluyver Jun 2, 2017

Member

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.

This comment has been minimized.

@damianavila

damianavila Jun 2, 2017

Author Member

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

This comment has been minimized.

@mpacer

mpacer Jun 2, 2017

Member

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.

This comment has been minimized.

@damianavila

damianavila Jun 2, 2017

Author Member

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

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

This comment has been minimized.

@takluyver

takluyver Jun 1, 2017

Member

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

This comment has been minimized.

@damianavila

damianavila Jun 1, 2017

Author Member

bit ugly

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

This comment has been minimized.

@damianavila

damianavila Jun 1, 2017

Author Member

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

@damianavila

This comment has been minimized.

Copy link
Member Author

damianavila commented Jun 1, 2017

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

@damianavila

This comment has been minimized.

Copy link
Member Author

damianavila commented Jun 1, 2017

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

@damianavila damianavila requested review from takluyver and mpacer Jun 1, 2017

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

This comment has been minimized.

@takluyver

takluyver Jun 2, 2017

Member

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.

This comment has been minimized.

@mpacer

mpacer Jun 2, 2017

Member

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

This comment has been minimized.

@damianavila

damianavila Jun 2, 2017

Author Member

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

mpacer left a comment

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?

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

This comment has been minimized.

@mpacer

mpacer Jun 2, 2017

Member

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

This comment has been minimized.

Copy link
Member Author

damianavila commented Jun 2, 2017

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

This comment has been minimized.

Copy link
Member

mpacer commented Jun 5, 2017

Ok I'll merge.

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

1 check passed

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

@mpacer mpacer added this to the 5.3 milestone Jun 5, 2017

@damianavila damianavila deleted the damianavila:upgrade_reveal branch Jun 7, 2017

@damianavila

This comment has been minimized.

Copy link
Member Author

damianavila commented Jun 7, 2017

Thanks @mpacer and @takluyver for you helpful review.

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