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

feat: Add PDF export options #277

Merged
merged 5 commits into from
Dec 10, 2019

Conversation

gquintana
Copy link
Contributor

@gquintana gquintana changed the title Add PDF export options feat: Add PDF export options Nov 25, 2019
README.adoc Outdated
@@ -877,6 +877,16 @@ a|Number of pixels to move the parallax background per slide
|<a valid CSS display mode>
|The display mode that will be used to show slides.
Defaults to *block*

|:revealjs_pdfSeparateFragments:
Copy link
Member

Choose a reason for hiding this comment

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

You need to use lowercase:

Suggested change
|:revealjs_pdfSeparateFragments:
|:revealjs_pdfseparatefragments:

See: 134bf3d

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are already revealjs_parallaxBackgroundHorizontal, revealjs_showNotes, etc. most options are revealjs_camelCaseOption. The only exceptions are revealjs_minscale and revealjs_maxscale. It doesn't make sense to me lowercase the option.
Are you sure you want me to change this?

Copy link
Member

Choose a reason for hiding this comment

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

There are already revealjs_parallaxBackgroundHorizontal, revealjs_showNotes, etc. most options are revealjs_camelCaseOption. The only exceptions are revealjs_minscale and revealjs_maxscale. It doesn't make sense to me lowercase the option.

It has changed recently, now all the attributes are lowercased (on master).

Are you sure you want me to change this?

Yes.

Although uppercase characters are permitted in an attribute entry (the place where an attribute is defined), the attribute name is converted to lowercase before being stored. The attribute name in an attribute reference is also converted to lowercase before the attribute is resolved. For example, URI, Uri and uRI are all treated as uri. (See issue #509 for a proposed change to this restriction). A best practice is to only use lowercase for letters in the name and avoid starting the name with a number.

https://asciidoctor.org/docs/user-manual/#attribute-restrictions

In short, you can define :revealjs_camelCaseOption: in your document but the attribute name will be converted to revealjs_camelcaseoption. So in the template we must use lowercase.
To avoid confusion, we also use lowercase in the documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mogztter On the branch master, it's still camel case, there's something I don't understand? Your link is pointing to asciidoctor manual, not asciidoctor-reveal.js.

Copy link
Member

Choose a reason for hiding this comment

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

Do you have a link where it's still in camel case? Here's what I get on the master branch:

Reveal.initialize({
// Display presentation control arrows
controls: #{to_boolean(attr 'revealjs_controls', true)},
// Help the user learn the controls by providing hints, for example by
// bouncing the down arrow when they first encounter a vertical slide
controlsTutorial: #{to_boolean(attr 'revealjs_controlstutorial', true)},
// Determines where controls appear, "edges" or "bottom-right"
controlsLayout: '#{attr 'revealjs_controlslayout', 'bottom-right'}',
// Visibility rule for backwards navigation arrows; "faded", "hidden"
// or "visible"
controlsBackArrows: '#{attr 'revealjs_controlsbackarrows', 'faded'}',
// Display a presentation progress bar
progress: #{to_boolean(attr 'revealjs_progress', true)},
// Display the page number of the current slide
slideNumber: #{to_valid_slidenumber(attr 'revealjs_slidenumber', false)},
// Control which views the slide number displays on
showSlideNumber: '#{attr 'revealjs_showslidenumber', 'all'}',
// Push each slide change to the browser history
history: #{to_boolean(attr 'revealjs_history', false)},
// Enable keyboard shortcuts for navigation
keyboard: #{to_boolean(attr 'revealjs_keyboard', true)},
// Enable the slide overview mode
overview: #{to_boolean(attr 'revealjs_overview', true)},
// Vertical centering of slides
center: #{to_boolean(attr 'revealjs_center', true)},
// Enables touch navigation on devices with touch input
touch: #{to_boolean(attr 'revealjs_touch', true)},
// Loop the presentation
loop: #{to_boolean(attr 'revealjs_loop', false)},
// Change the presentation direction to be RTL
rtl: #{to_boolean(attr 'revealjs_rtl', false)},
// Randomizes the order of slides each time the presentation loads
shuffle: #{to_boolean(attr 'revealjs_shuffle', false)},
// Turns fragments on and off globally
fragments: #{to_boolean(attr 'revealjs_fragments', true)},
// Flags whether to include the current fragment in the URL,
// so that reloading brings you to the same fragment position
fragmentInURL: #{to_boolean(attr 'revealjs_fragmentinurl', false)},
// Flags if the presentation is running in an embedded mode,
// i.e. contained within a limited portion of the screen
embedded: #{to_boolean(attr 'revealjs_embedded', false)},
// Flags if we should show a help overlay when the questionmark
// key is pressed
help: #{to_boolean(attr 'revealjs_help', true)},
// Flags if speaker notes should be visible to all viewers
showNotes: #{to_boolean(attr 'revealjs_shownotes', false)},
// Global override for autolaying embedded media (video/audio/iframe)
// - null: Media will only autoplay if data-autoplay is present
// - true: All media will autoplay, regardless of individual setting
// - false: No media will autoplay, regardless of individual setting
autoPlayMedia: #{attr 'revealjs_autoplaymedia', 'null'},
// Number of milliseconds between automatically proceeding to the
// next slide, disabled when set to 0, this value can be overwritten
// by using a data-autoslide attribute on your slides
autoSlide: #{attr 'revealjs_autoslide', 0},
// Stop auto-sliding after user input
autoSlideStoppable: #{to_boolean(attr 'revealjs_autoslidestoppable', true)},
// Use this method for navigation when auto-sliding
autoSlideMethod: #{attr 'revealjs_autoslidemethod', 'Reveal.navigateNext'},
// Specify the average time in seconds that you think you will spend
// presenting each slide. This is used to show a pacing timer in the
// speaker view
defaultTiming: #{attr 'revealjs_defaulttiming', 120},
// Enable slide navigation via mouse wheel
mouseWheel: #{to_boolean(attr 'revealjs_mousewheel', false)},
// Hides the address bar on mobile devices
hideAddressBar: #{to_boolean(attr 'revealjs_hideaddressbar', true)},
// Opens links in an iframe preview overlay
// Add `data-preview-link` and `data-preview-link="false"` to customise each link
// individually
previewLinks: #{to_boolean(attr 'revealjs_previewlinks', false)},
// Transition style (e.g., none, fade, slide, convex, concave, zoom)
transition: '#{attr 'revealjs_transition', 'slide'}',
// Transition speed (e.g., default, fast, slow)
transitionSpeed: '#{attr 'revealjs_transitionspeed', 'default'}',
// Transition style for full page slide backgrounds (e.g., none, fade, slide, convex, concave, zoom)
backgroundTransition: '#{attr 'revealjs_backgroundtransition', 'fade'}',
// Number of slides away from the current that are visible
viewDistance: #{attr 'revealjs_viewdistance', 3},
// Parallax background image (e.g., "'https://s3.amazonaws.com/hakim-static/reveal-js/reveal-parallax-1.jpg'")
parallaxBackgroundImage: '#{attr 'revealjs_parallaxbackgroundimage', ''}',
// Parallax background size in CSS syntax (e.g., "2100px 900px")
parallaxBackgroundSize: '#{attr 'revealjs_parallaxbackgroundsize', ''}',
// Number of pixels to move the parallax background per slide
// - Calculated automatically unless specified
// - Set to 0 to disable movement along an axis
parallaxBackgroundHorizontal: #{attr 'revealjs_parallaxbackgroundhorizontal', 'null'},
parallaxBackgroundVertical: #{attr 'revealjs_parallaxbackgroundvertical', 'null'},
// The display mode that will be used to show slides
display: '#{attr 'revealjs_display', 'block'}',
// The "normal" size of the presentation, aspect ratio will be preserved
// when the presentation is scaled to fit different resolutions. Can be
// specified using percentage units.
width: #{attr 'revealjs_width', 960},
height: #{attr 'revealjs_height', 700},
// Factor of the display size that should remain empty around the content
margin: #{attr 'revealjs_margin', 0.1},
// Bounds for smallest/largest possible scale to apply to content
minScale: #{attr 'revealjs_minscale', 0.2},
maxScale: #{attr 'revealjs_maxscale', 1.5},
// Optional libraries used to extend on reveal.js
dependencies: [
{ src: '#{revealjsdir}/lib/js/classList.js', condition: function() { return !document.body.classList; } },
#{(document.attr? 'source-highlighter', 'highlightjs') ? "{ src: '#{revealjsdir}/plugin/highlight/highlight.js', async: true, callback: function() { hljs.initHighlightingOnLoad(); } }," : nil}
#{(attr? 'revealjs_plugin_zoom', 'disabled') ? "" : "{ src: '#{revealjsdir}/plugin/zoom-js/zoom.js', async: true }," }
#{(attr? 'revealjs_plugin_notes', 'disabled') ? "" : "{ src: '#{revealjsdir}/plugin/notes/notes.js', async: true }," }
#{(attr? 'revealjs_plugin_marked', 'enabled') ? "{ src: '#{revealjsdir}/plugin/markdown/marked.js', condition: function() { return !!document.querySelector( '[data-markdown]' ); } }," : "" }
#{(attr? 'revealjs_plugin_markdown', 'enabled') ? "{ src: '#{revealjsdir}/plugin/markdown/markdown.js', condition: function() { return !!document.querySelector( '[data-markdown]' ); } }," : "" }
#{(attr? 'revealjs_plugin_pdf', 'enabled') ? "{ src: '#{revealjsdir}/plugin/print-pdf/print-pdf.js', async: true }," : "" }
#{(attr? 'revealjs_plugins') ? File.read(attr('revealjs_plugins', '')) : ""}
],
#{(attr? 'revealjs_plugins_configuration') ? File.read(attr('revealjs_plugins_configuration', '')) : ""}

Your link is pointing to asciidoctor manual, not asciidoctor-reveal.js.

Yes but Asciidoctor Reveal.js is using Asciidoctor to resolve attributes so we must comply with the above rule.

Copy link
Contributor Author

@gquintana gquintana Dec 5, 2019

Choose a reason for hiding this comment

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

Here https://raw.githubusercontent.com/asciidoctor/asciidoctor-reveal.js/master/README.adoc
The documentation doesn't match the code indeed.
I changed casing.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see... technically it works because revealjs_camelCaseOption will be resolved to revealjs_camelcaseoption.

@obilodeau Do you have an opinion? Should we also use lowercase in the documentation?

Copy link
Member

@obilodeau obilodeau Dec 10, 2019

Choose a reason for hiding this comment

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

Yeah, at first I created a commit that fixed the case for all the doc but then I realized: why bother? It worked for people before and it will continue working. If attributes ever become case-sensitive then we will fix it and in the meantime, it is more readable than without casing.

README.adoc Outdated
|In PDF export, put each fragment on a separate page.
Defaults to *true*

|:revealjs_pdfMaxPagesPerSlide:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
|:revealjs_pdfMaxPagesPerSlide:
|:revealjs_pdfmaxpagesperslide:

@@ -229,6 +229,12 @@ html lang=(attr :lang, 'en' unless attr? :nolang)
minScale: #{attr 'revealjs_minscale', 0.2},
maxScale: #{attr 'revealjs_maxscale', 1.5},

// PDF Export Options
// Put each fragment on a separate page
pdfSeparateFragments: #{to_boolean(attr 'revealjs_pdfSeparateFragments', true)},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pdfSeparateFragments: #{to_boolean(attr 'revealjs_pdfSeparateFragments', true)},
pdfSeparateFragments: #{to_boolean(attr 'revealjs_pdfseparatefragments', true)},

// Put each fragment on a separate page
pdfSeparateFragments: #{to_boolean(attr 'revealjs_pdfSeparateFragments', true)},
// For slides that do not fit on a page, max number of pages
pdfMaxPagesPerSlide: #{attr 'revealjs_pdfMaxPagesPerSlide', 1},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pdfMaxPagesPerSlide: #{attr 'revealjs_pdfMaxPagesPerSlide', 1},
pdfMaxPagesPerSlide: #{attr 'revealjs_pdfmaxpagesperslide', 1},

@ggrossetie
Copy link
Member

Tests are failing because the expected output changed: https://travis-ci.org/asciidoctor/asciidoctor-reveal.js/jobs/616847269?utm_medium=notification&utm_source=github_status

Ideally we should also add tests on this new feature to make sure that pdfMaxPagesPerSlide has the correct value when revealjs_pdfmaxpagesperslide is defined. Same thing applies for pdfSeparateFragments.

Copy link
Member

@ggrossetie ggrossetie left a comment

Choose a reason for hiding this comment

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

👍

@obilodeau obilodeau merged commit 143b286 into asciidoctor:master Dec 10, 2019
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