Bundler friendliness (part II) - Repackage print style sheets #2633

quilicicf · 2020-03-10T13:22:15Z

What this PR does

It applies paper.css and pdf.css conditionally (depending on whether the query parameter ?print-pdf is defined) so they can both be loaded in the presentation without adding a script.

The style sheets can now be both imported and only apply on the following conditions:

paper.css: only applied when printing (media query) and the query parameter is NOT present
pdf.css: only applied when the query parameter is present

Question

Shouldn't the pdf style sheet also be applied only when printing? I believe so but I'm not certain.

⚠️ The current version is a bit buggy

I didn't validate using the test suite because it is commented out (see gulpfile).

I did inspect the result on the page demo.html though and found a nasty bug that I don't have the time to locate right now.

The first slide when printing to pdf with black theme follows the theme correctly but all the other slides have a white background.

I'll try to fix this ASAP, don't merge in the meantime :-)

quilicicf · 2020-03-11T13:02:46Z

Soooo, I have checked the CSS bug for PDF printing and it appears to be in the dev branch too.
For an unknown reason, the option background graphics in Chrome print preview was auto-activated when testing on your branch but not on mine 🤔

Turns out the backgrounds are OK on both branches when turning it on.

There's one remaining issue (on both branches too), a page is added when printing to PDF, and given it has no content, it also has no background => it is white even with dark themes.

quilicicf · 2020-03-11T13:55:19Z

Dug a bit further. The additional page in the PDF is actually added because there's a tiny DOM element that gets added at the end of the presentation that looks like this:

<div class="aria-status" aria-live="polite" aria-atomic="true" style="position: absolute; height: 1px; width: 1px; overflow: hidden; clip: rect(1px, 1px, 1px, 1px);">
    Presentation's first slide's text here
</div>

Switching the height to 0 removes the additional page.

quilicicf · 2020-03-12T12:04:58Z

I have added a commit to hide the status element off-screen.
It works for me now.
Can you check that I didn't wreck anything @hakimel ?

hakimel · 2020-03-12T12:50:40Z

js/reveal.js

@@ -368,7 +370,7 @@ export default function( revealElement, options ) {
 		// Limit the size of certain elements to the dimensions of the slide
 		createStyleSheet( '.reveal section>img, .reveal section>video, .reveal section>iframe{max-width: '+ slideWidth +'px; max-height:'+ slideHeight +'px}' );

-		document.body.classList.add( 'print-pdf' );
+		document.querySelector('html').classList.add( 'print-pdf' );


This should use document.documentElement instead of querySelector('html'), to match how the rest of reveal.js accesses it.

hakimel · 2020-03-12T12:52:29Z

js/reveal.js

@@ -292,6 +292,8 @@ export default function( revealElement, options ) {
 			statusElement.style.position = 'absolute';
 			statusElement.style.height = '1px';
 			statusElement.style.width = '1px';
+			statusElement.style.top = '-1px';
+			statusElement.style.left = '-1px';


The status element is used for accessibility—it calls out text to a screen reader. It serves no purpose in PDF exports though, so maybe we can add .aria-status { display: none; } to the pdf print sheet instead of changing it here?

Yup, seems reasonable. I'll do that.

quilicicf · 2020-03-12T15:17:30Z

Comments taken into account.
Another thing I saw is that the comments stay in the CSS after SASS compilation. And they jump to the top of the file, meaning they have no context.

Shouldn't they disappear in the CSS? I can configure the gulp task but I want to be sure what's expected here.

Example, pdf.css starts like this:

/* Copyright... */

html.print-pdf {
  /* Remove any elements not needed in print. */
  /* Slide backgrounds are placed inside of their slide when exporting to PDF */
  /* Display slide speaker notes when 'showNotes' is enabled */
  /* Layout option which makes notes appear on a separate page */
  /* Display slide numbers when 'slideNumber' is enabled */
  /* This accessibility tool is not useful in PDF and breaks it visually */ }

I don't think Reveal should add the comments in "production" files, should it?

hakimel · 2020-03-12T18:17:11Z

PR is looking good so I'm gonna start testing.

I'll take a look at the build to see why it's not removing those comments. The whole dev branch—including the new gulp build—is undergoing some pretty big changes so it's not in a stable place yet :)

hakimel · 2020-03-12T18:19:09Z

Also one thought that springs to mind looking at this is why don't we just bundle the pdf and paper styles inside of reveal.css to avoid the separate imports? I'll poke around and see if that's feasible while testing.

quilicicf · 2020-03-13T10:20:07Z

I can definitely try to merge them if that helps, just tell me.
I was also wondering if the pdf thing should not be also in the print media query ?
I'm not sure it serves a purpose to wreck the style of the presentation when the ?print-pdf parameter is there and the user is not trying to print.

hakimel · 2020-03-13T14:20:59Z

Let's avoid wrapping the PDF styles in a media query. There are multiple scripts out there for exporting reveal.js presentations to PDF, including our implementation at Slides.com, that may be affected by that.

I'm experimenting with building the print styles into the main stylesheet—will let you know how it goes.

quilicicf · 2020-03-13T16:40:51Z

OK, good luck :)

hakimel · 2020-03-13T18:57:59Z

Merged with two changes;

Paper and PDF print styles are now baked into reveal.css 🥳 Updated README accordingly.
The print to paper styles were severely outdated so I updated them.

quilicicf · 2020-03-15T16:46:41Z

Noice. Will attack the third part of my evil plan next week.
I need to make the speaker notes plugin bundler-friendly which won't be a piece of cake.
I'll share my ideas first as this is gonna be more complex by far than the two first parts IMO.

…imel#2633

quilicicf mentioned this pull request Mar 10, 2020

Bundler-friendliness #2526

Closed

quilicicf force-pushed the dev_repackagePrintStyleSheets branch from 138e2f3 to eae6704 Compare March 11, 2020 12:09

Package paper and pdf style sheets and apply them conditionally

499dc68

quilicicf force-pushed the dev_repackagePrintStyleSheets branch from eae6704 to 499dc68 Compare March 11, 2020 12:10

Hide the status element off-screen to avoid additional page in PDF print

6564950

hakimel reviewed Mar 12, 2020

View reviewed changes

Remove accessible slide status when printing to PDF

ffac5fe

quilicicf mentioned this pull request Mar 13, 2020

Make Reveal more bundler-friendly #2524

Open

hakimel added a commit that referenced this pull request Mar 13, 2020

print styles are now built into reveal.css, revamped print styles #2633

d918d75

hakimel merged commit 92e3a36 into hakimel:dev Mar 13, 2020

R0bes pushed a commit to R0bes/Terraform-Presentation that referenced this pull request Jun 7, 2021

print styles are now built into reveal.css, revamped print styles hak…

9b8b9d3

…imel#2633

srwohl pushed a commit to srwohl/phantom-pres that referenced this pull request Sep 2, 2024

print styles are now built into reveal.css, revamped print styles hak…

315bce6

…imel#2633

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bundler friendliness (part II) - Repackage print style sheets #2633

Bundler friendliness (part II) - Repackage print style sheets #2633

quilicicf commented Mar 10, 2020

quilicicf commented Mar 11, 2020

quilicicf commented Mar 11, 2020

quilicicf commented Mar 12, 2020

hakimel Mar 12, 2020

quilicicf Mar 12, 2020

hakimel Mar 12, 2020

quilicicf Mar 12, 2020

quilicicf commented Mar 12, 2020

hakimel commented Mar 12, 2020

hakimel commented Mar 12, 2020

quilicicf commented Mar 13, 2020

hakimel commented Mar 13, 2020

quilicicf commented Mar 13, 2020

hakimel commented Mar 13, 2020

quilicicf commented Mar 15, 2020

Bundler friendliness (part II) - Repackage print style sheets #2633

Bundler friendliness (part II) - Repackage print style sheets #2633

Conversation

quilicicf commented Mar 10, 2020

What this PR does

Question

⚠️ The current version is a bit buggy

quilicicf commented Mar 11, 2020

quilicicf commented Mar 11, 2020

quilicicf commented Mar 12, 2020

hakimel Mar 12, 2020

Choose a reason for hiding this comment

quilicicf Mar 12, 2020

Choose a reason for hiding this comment

hakimel Mar 12, 2020

Choose a reason for hiding this comment

quilicicf Mar 12, 2020

Choose a reason for hiding this comment

quilicicf commented Mar 12, 2020

hakimel commented Mar 12, 2020

hakimel commented Mar 12, 2020

quilicicf commented Mar 13, 2020

hakimel commented Mar 13, 2020

quilicicf commented Mar 13, 2020

hakimel commented Mar 13, 2020

quilicicf commented Mar 15, 2020