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

Update reveal, fix scrolling and add transition option #600

Merged
merged 6 commits into from
Jun 5, 2017
Merged

Update reveal, fix scrolling and add transition option #600

merged 6 commits into from
Jun 5, 2017

Conversation

damianavila
Copy link
Member

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

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

Choose a reason for hiding this comment

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

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

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 did not try to use CSS calc... will take a look in your suggestions.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 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}}";
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

bit ugly

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

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 change my mind and I went with the json_dumps approach (and simplify the structure a little bit).

@damianavila
Copy link
Member Author

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

@damianavila
Copy link
Member Author

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

{
width: 6px;
height: 6px;
}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@damianavila damianavila Jun 2, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member

@mpacer mpacer left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

mpacer commented Jun 5, 2017

Ok I'll merge.

@mpacer mpacer merged commit 760cd49 into jupyter:master Jun 5, 2017
@mpacer mpacer added this to the 5.3 milestone Jun 5, 2017
@damianavila damianavila deleted the upgrade_reveal branch June 7, 2017 13:14
@damianavila
Copy link
Member Author

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants