resolves #326 add grid layout #332

ggrossetie · 2020-02-10T22:26:48Z

resolves #326

templates/asciidoctor-compatibility.css

templates/section.html.slim

ggrossetie · 2020-02-10T23:13:22Z

@obilodeau I will fix the doctests but first I want to make sure that you are OK with the changes.

obilodeau · 2020-02-11T03:18:29Z

I'm going to have to give this a serious try before I can conclude. With existing decks where I wish I had that.

Try adding vertical slides to your example too, just to validate:

== Languages

We will go over languages

=== Java
// TODO code snippet on the left, bullets saying why it's awesome on the right

=== Kotlin
// TODO code snippet on the left, bullets saying why it's awesome on the right

examples/grid-layout.adoc

obilodeau · 2020-02-11T15:50:24Z

Merged with master to integrate Use content_for and yield_content to remove code duplication #333 changes and resolve conflict once
Added vertical slides example
Found a bug in my merge where div.slide-content was not created on the top of a vertical slides series

I'm still testing decks with it

ggrossetie · 2020-02-11T17:42:30Z

examples/grid-layout.adoc

+[.column]
+--
+* Lisp-like
+* Not sure why people like it


Are you trolling Clojure? 😆

Yeah, you should probably change that. I didn't know what to say about Clojure and I wanted two bullets. I haven't done Java or any JVM-language for 10+ years.

ggrossetie · 2020-02-11T17:44:20Z

Found a bug in my merge where div.slide-content was not created on the top of a vertical slides series

Did you fix it?

I'm still testing decks with it

Let me know if it looks fine, I will update the doctests to add the wrapping <div class="slide-content">.

obilodeau · 2020-02-11T19:05:50Z

Did you fix it?

Yes I did in 275e91e. Sorry if it wasn't clear.

obilodeau · 2020-02-11T20:26:42Z

There is a regression in examples/video.adoc.

Before change:

After change:

ggrossetie · 2020-02-11T21:13:50Z

Nice catch!
I will try to add visual tests to catch regressions more easily.

obilodeau · 2020-02-12T02:48:07Z

After some messing around, it is clear that it is the stretch class that causes problems. Slide 3 of examples/source-callouts.html also has a problem.

stretch is special, in the sense that it's not changing how things look with CSS. It relies on special reveal.js JavaScript. You can read about the feature here: https://github.com/hakimel/reveal.js/#stretching-elements

Note:

Limitations:

Only direct descendants of a slide section can be stretched

Only one descendant per slide section can be stretched

From past observation, I would say that it injects maximum width and height in pixels directly in the style= HTML attribute of the HTML tag where the stretch class is present.

It is applied by default to videos because who would want a small video box in a slide.

ggrossetie · 2020-02-12T06:56:04Z

We should probably recreate this feature using CSS or workaround the "only direct descendant" limitation.

ggrossetie · 2020-02-12T19:55:24Z

Using flexbox "everywhere" could add more flexibility but it does require a few changes and probably a few breaking changes since stretch/grow need to be on the parent (if we want to use CSS and not JavaScript).

Anyway, I will try to fix this issue with the least amount of work. We can then decide if we want to switch to flexbox/grid or stick with the absolute/center positioning.

ggrossetie · 2020-02-12T20:53:11Z

stretch element sizing is done in the layoutSlideContents function which is called in the layout function which is called when the window is resized.

But layout function is also called when the overview is activated/deactivated or when we go to the next/previous slide. So I think that registering a listener to size the nested stretch elements won't be enough 🤔

obilodeau · 2020-02-12T21:34:13Z

I'm surprised that this problem exists. I mean no one ever used two-columns layouts in reveal.js!?

Can it be done as a plugin instead of monkey-patching? That would work as well.

ggrossetie · 2020-02-12T22:16:00Z

I'm surprised that this problem exists. I mean no one ever used two-columns layouts in reveal.js!?

It can definitely be done without using a wrapping <div> (and flexbox) but it will be a bit "limited" in comparison.
I think we should implement this feature without using flexbox. I don't want to rush a half-baked implementation based on flexbox.
Please note that switching to flexbox will most likely require a major version (ie. 5.x)

Can it be done as a plugin instead of monkey-patching? That would work as well.

Maybe.

obilodeau · 2020-02-13T01:12:02Z

I'm not sure about going without it... While we are here shouldn't we get this right? Besides the stretch issue this seems like a great way to solve the basic layouts for presentations fairly easily.

Regarding the stretch issue, we control everything. Isn't there a way to maximize a single flex div? We put the stretch attribute on videos and we can even intercept the attribute and do something else.

Even bolder, couldn't we add an onChange() event on the big div and propagate the values set by reveal.js to the first stretch child? Brainstorming here

ggrossetie · 2020-02-13T08:00:43Z

Let me try to summarize.
The main issue is that height: 100% does not work.
I believe it's because the parent has an absolute positioning and/or because reveal.js is dynamically updating the height, width and zoom on the parent.
Anyway, height:100% on the <section> element is working as expected. But does not work on child elements.

Let's take an example:

Simplified HTML

<body>
  <div class="reveal slide">
    <div class="slides" style="width: 960px; height: 700px; scale(1);">
      <section>                                    <!-- height:100% = 700px + 40px (padding) = OK -->
        <h2>Stretched Callout</h2>
        <div class="slide-content">                <!-- height:100% = 700px = KO -->
          <div class="listingblock stretch">       <!-- height:100% = 700px = KO -->
          </div>
          <div class="colist arabic">
          </div>
        </div>
      </section>
    </div>
  </div>
</body>

Here, the element "slide-content" has a 100% height of 700px but it's wrong because it ignores the <h2> element height. Even worse, the "listingblock" element also has a 100% height of 700px. In this particular case, it means that the "colist" element will be pushed outside of the slide 😱

Regarding the stretch issue, we control everything. Isn't there a way to maximize a single flex div? We put the stretch attribute on videos and we can even intercept the attribute and do something else.

Yes we can.
To workaround this issue, I can use flex-grow but as far as I know I need to apply the flex-grow: 1 on each parent:

<section> = height:100% + flex-grow:1
<div class="slide-content"> = flex-grow:1
<div class="listingblock stretch"> = flex-grow:1

The main issue is that we need to use display: flex on the section. This value is controlled by a reveal.js option named display.
At this point, I thought, let's just ignore this value and just use display: flex everywhere, see: master...Mogztter:issue-326-columns-layout-flex
It's working but it introduces a few changes as it does not rely on JavaScript anymore. I guess we could use a bit of JavaScript to propagate the stretch value on the parent elements. For now, I'm using a grow role on the slide.

One limitation with this solution is that the listingblock has no maximum height anymore. So if the content is higher than the remaining height available it won't show a scrollbar but exceed the slide height:

In this case, I think using JavaScript to set a max-height with is the only way to fix it.

obilodeau · 2020-02-13T14:04:41Z

Does it negatively affects?

Speaker mode (press s)
Overview mode (press Esc)
PDF output (add ?print-pdf to the URL and print without margins)

I wonder why the config.display feature of reveal.js is there...

ggrossetie · 2020-02-13T14:18:15Z

Does it negatively affects?

Not sure but since we need JavaScript to compute the max height, I will try to workaround this limitation "Only direct descendants of a slide section can be stretched".

I wonder why the config.display feature of reveal.js is there...

I believe that @mojavelinux added it to configure a flex display. The display value is set (via JavaScript) on the section element, so it adds flexibility.
In my branch, I force the value to "flex": master...Mogztter:issue-326-columns-layout-flex#diff-38f9a53261d123f9921bdc0090cc4912R224

obilodeau · 2020-02-13T15:45:57Z

I wonder why the config.display feature of reveal.js is there...

I believe that @mojavelinux added it to configure a flex display. The display value is set (via JavaScript) on the section element, so it adds flexibility.
In my branch, I force the value to "flex": master...Mogztter:issue-326-columns-layout-flex#diff-38f9a53261d123f9921bdc0090cc4912R224

/me did a git blame upstream.

Oh my god, wow! This change really was introduced by @mojavelinux. Probably before he started using bespoke 😮 I didn't know.

ggrossetie · 2020-02-13T16:39:50Z

It's fairly easy to remove the limitation so my guess is that the decision was made for performance reasons. I'm asking upstream about a possible solution.

ggrossetie · 2020-02-13T16:55:18Z

Can you please give it a try @obilodeau?

obilodeau · 2020-02-13T19:41:16Z

I'll try later tonight. Thanks!

obilodeau · 2020-02-14T05:34:53Z

Video

On the first load of the converted examples/video.html, when I get to the vimeo slide there's a glitch:

There's an empty space at the top. On other videos this pushes the video slightly out of the screen. I will add a test case in this deck where this is worse. Using the embedded reveal.js video player.

If I manually resize the browser by dragging around the window then replacing it full screen the glitch is gone:

I was able to confirm the same glitch on a stretched image in a private slide deck.

Speaker Notes

Were ok

PDF

With ?print-pdf, images with the .stretch class disappeared from PDF output. However, that problem is familiar to me. Anything larger than the page would usually get removed in print pdf mode. I think if you resolve the video stretch issue it will get rid of that one too.

Overview mode

Works fine

Misc

I noticed that YouTube embeds didn't autoplay but I had the same problem with master so we can ignore for the sake of this PR.

Results

Besides the above glitches, I tested several decks and they were fine! 👍

obilodeau · 2020-02-14T05:43:33Z

Here's the other example. Available with: examples/video.html#/_remote_video_file_auto_sized

Glitched:

After a manual resize, not glitched:

Hopefully you'll be able to track this one down. Good luck!

ggrossetie · 2020-02-14T12:35:35Z

I thought it was unnecessary to resize the stretch elements when the slide changed but I guess I was wrong...
I've just added an event listener, it seems to fix this issue.

obilodeau · 2020-02-14T14:17:59Z

Great!

When testing, I thought I found a problem with PDF rendering on a large slide deck, so I attempted to make a simpler case where it is reproduced. However, in my simpler case, I couldn't reproduce.

However, I found the problem.

It boils down to the theme that I made. I still use this CSS from another theme:

/* Ensure certain elements are never larger than the slide itself */
.reveal img,
.reveal video,
.reveal iframe {
  max-width: 95%;
  max-height: 95%;
  }

But if I maximize the height of an image (with .stretch), it gets slightly off (even with 95% max) so it gets bumped out in print mode. Reducing this to 90% in DevTools fixed it. This is probably due to the font and the spacing on top in my theme which requires this change. Anyway, long story short, the issue seems to be in my theme.

To me this seems to be a 👍. How do you feel about this PR @Mogztter?

ggrossetie · 2020-02-14T17:39:40Z

But if I maximize the height of an image (with .stretch), it gets slightly off (even with 95% max) so it gets bumped out in print mode. Reducing this to 90% in DevTools fixed it. This is probably due to the font and the spacing on top in my theme which requires this change. Anyway, long story short, the issue seems to be in my theme.

It sounds like a limitation of the original getRemainingHeight function that do not or cannot take everything into account.

To me this seems to be a +1.

Should I rebase on master?

How do you feel about this PR @Mogztter?

I'm not 100% satisfied because we still need to use JavaScript but I think it's the best we can do for now. In my opinion, the column definition is quite simple and will probably be enough in the majority of cases. So I think we should go for it!

obilodeau

Add doctest and this is good to go!

You can document the feature in the README if you are willing but if you don't I'll do it.

obilodeau · 2020-02-14T17:57:51Z

Thanks by the way! This is something I wanted for a while and I wasn't sure how to tackle it in a flexible manner. Pun intended.

A very nice piece of work. Hopefully, upstream will be kind to us and we will be able to remove that JavaScript patch.

ggrossetie · 2020-02-14T18:40:41Z

Should I remove the col2 role in the source-emphasis.html example?

Add doctest and this is good to go!

I found a few issues while updating doctests 😉

You can document the feature in the README if you are willing but if you don't I'll do it.

Ok!

Thanks by the way! This is something I wanted for a while and I wasn't sure how to tackle it in a flexible manner. Pun intended.

😊

A very nice piece of work. Hopefully, upstream will be kind to us and we will be able to remove that JavaScript patch.

Yep, that would be nice 👍

obilodeau · 2020-02-14T19:02:20Z

Should I remove the col2 role in the source-emphasis.html example?

Yes, replace it with this new stuff and drop its customcss file.

…d limitation

examples/grid-layout.adoc

ggrossetie · 2020-02-15T10:10:57Z

Let's go 🚀 🚀 🚀

ggrossetie commented Feb 10, 2020

View reviewed changes

templates/asciidoctor-compatibility.css Show resolved Hide resolved

ggrossetie commented Feb 10, 2020

View reviewed changes

templates/section.html.slim Outdated Show resolved Hide resolved

obilodeau reviewed Feb 11, 2020

View reviewed changes

examples/grid-layout.adoc Show resolved Hide resolved

obilodeau added a commit to ggrossetie/asciidoctor-reveal.js that referenced this pull request Feb 11, 2020

Fixed conflict with asciidoctor#333 in asciidoctor#332

fa6a079

ggrossetie commented Feb 11, 2020

View reviewed changes

ggrossetie requested a review from obilodeau February 11, 2020 17:42

ggrossetie force-pushed the issue-326-columns-layout branch from 3e2e9c8 to 7c7cb0d Compare February 13, 2020 16:53

ggrossetie force-pushed the issue-326-columns-layout branch 4 times, most recently from d97354d to 3772787 Compare February 14, 2020 17:53

obilodeau approved these changes Feb 14, 2020

View reviewed changes

ggrossetie force-pushed the issue-326-columns-layout branch from 3772787 to 2474dc2 Compare February 14, 2020 19:15

ggrossetie and others added 5 commits February 14, 2020 20:29

resolves asciidoctor#326 add grid layout

48f93e6

Added vertical slides example

b635709

Added a video viewer test to examples/video.adoc

9d632ca

Remove the only direct descendants of a slide section can be stretche…

661b034

…d limitation

Fix doctest

3c5f0fc

ggrossetie force-pushed the issue-326-columns-layout branch from 661ab0d to d56f553 Compare February 14, 2020 19:30

Use columns and column role in the source-emphasis.html example

b99fc87

ggrossetie force-pushed the issue-326-columns-layout branch from d56f553 to b99fc87 Compare February 14, 2020 19:31

obilodeau reviewed Feb 15, 2020

View reviewed changes

examples/grid-layout.adoc Show resolved Hide resolved

obilodeau merged commit 02ebdd2 into asciidoctor:master Feb 15, 2020

obilodeau added this to the 4.0.0 milestone Feb 15, 2020

ggrossetie deleted the issue-326-columns-layout branch February 15, 2020 10:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resolves #326 add grid layout #332

resolves #326 add grid layout #332

ggrossetie commented Feb 10, 2020

ggrossetie commented Feb 10, 2020

obilodeau commented Feb 11, 2020

obilodeau commented Feb 11, 2020

ggrossetie Feb 11, 2020

obilodeau Feb 11, 2020

ggrossetie commented Feb 11, 2020

obilodeau commented Feb 11, 2020

obilodeau commented Feb 11, 2020

ggrossetie commented Feb 11, 2020

obilodeau commented Feb 12, 2020

ggrossetie commented Feb 12, 2020

ggrossetie commented Feb 12, 2020

ggrossetie commented Feb 12, 2020

obilodeau commented Feb 12, 2020

ggrossetie commented Feb 12, 2020

obilodeau commented Feb 13, 2020

ggrossetie commented Feb 13, 2020 •

edited

Loading

obilodeau commented Feb 13, 2020

ggrossetie commented Feb 13, 2020

obilodeau commented Feb 13, 2020

ggrossetie commented Feb 13, 2020

ggrossetie commented Feb 13, 2020

obilodeau commented Feb 13, 2020

obilodeau commented Feb 14, 2020 •

edited

Loading

obilodeau commented Feb 14, 2020

ggrossetie commented Feb 14, 2020

obilodeau commented Feb 14, 2020

ggrossetie commented Feb 14, 2020

obilodeau left a comment

obilodeau commented Feb 14, 2020

ggrossetie commented Feb 14, 2020

obilodeau commented Feb 14, 2020

ggrossetie commented Feb 15, 2020

resolves #326 add grid layout #332

resolves #326 add grid layout #332

Conversation

ggrossetie commented Feb 10, 2020

ggrossetie commented Feb 10, 2020

obilodeau commented Feb 11, 2020

obilodeau commented Feb 11, 2020

ggrossetie Feb 11, 2020

Choose a reason for hiding this comment

obilodeau Feb 11, 2020

Choose a reason for hiding this comment

ggrossetie commented Feb 11, 2020

obilodeau commented Feb 11, 2020

obilodeau commented Feb 11, 2020

ggrossetie commented Feb 11, 2020

obilodeau commented Feb 12, 2020

ggrossetie commented Feb 12, 2020

ggrossetie commented Feb 12, 2020

ggrossetie commented Feb 12, 2020

obilodeau commented Feb 12, 2020

ggrossetie commented Feb 12, 2020

obilodeau commented Feb 13, 2020

ggrossetie commented Feb 13, 2020 • edited Loading

obilodeau commented Feb 13, 2020

ggrossetie commented Feb 13, 2020

obilodeau commented Feb 13, 2020

ggrossetie commented Feb 13, 2020

ggrossetie commented Feb 13, 2020

obilodeau commented Feb 13, 2020

obilodeau commented Feb 14, 2020 • edited Loading

Video

Speaker Notes

PDF

Overview mode

Misc

Results

obilodeau commented Feb 14, 2020

ggrossetie commented Feb 14, 2020

obilodeau commented Feb 14, 2020

ggrossetie commented Feb 14, 2020

obilodeau left a comment

Choose a reason for hiding this comment

obilodeau commented Feb 14, 2020

ggrossetie commented Feb 14, 2020

obilodeau commented Feb 14, 2020

ggrossetie commented Feb 15, 2020

ggrossetie commented Feb 13, 2020 •

edited

Loading

obilodeau commented Feb 14, 2020 •

edited

Loading