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

resolves #370 add support for reveal.js 4.x #427

Closed
wants to merge 17 commits into from

Conversation

edl7878
Copy link
Contributor

@edl7878 edl7878 commented Jun 6, 2021

Reveal.js 4.x assets path is varied from 3.9.x, in this pull request, asciidoctor-reveal.js code is modified:

  • paths are directed to the right place
  • since print related CSSes are included in reveal.css in reveal.js 4.x, print css injections are moved out in code base

@edl7878 edl7878 mentioned this pull request Jun 6, 2021
@ggrossetie
Copy link
Member

Thanks @edl7878

Since you've made some changes on the templates, you will need to update the tests as well.

@ggrossetie
Copy link
Member

Could you please also update revea.js version in:

"reveal.js": "3.9.2"

@edl7878
Copy link
Contributor Author

edl7878 commented Jun 8, 2021

Thanks @edl7878

Since you've made some changes on the templates, you will need to update the tests as well.

How can I update the test?

@ggrossetie
Copy link
Member

@edl7878 you can run tests locally using:

bundle exec rake build
bundle exec rake test

You will need to update the HTML files in https://github.com/asciidoctor/asciidoctor-reveal.js/tree/master/test/doctest

For instance, in test/doctest/custom-layout.html, you will need to fix two lines:

<script src="reveal.js/js/reveal.js"></script>

It should be <script src="reveal.js/dist/reveal.js"></script>

{ src: 'reveal.js/plugin/zoom-js/zoom.js', async: true },

It should be { src: 'reveal.js/plugin/zoom/zoom.js', async: true },

And here's the relevant test failure (taken from GitHub Action log):

(See full trace by running task with --trace)
✗  Failure: custom-layout:custom-layout
   The three different ways to hide slide titles.

          </div>
        </section>
      </div>
   E  <script src="reveal.js/js/reveal.js"></script><script>
   A  <script src="reveal.js/dist/reveal.js"></script><script>
        Array.prototype.slice.call(document.querySelectorAll('.slides section')).forEach(function(slide) {
          if (slide.getAttribute('data-background-color')) return;
          // user needs to explicitly say he wants CSS color to override otherwise we might break custom css or theme (#226)
        
          // Optional libraries used to extend on reveal.js
          dependencies: [
   E          { src: 'reveal.js/plugin/zoom-js/zoom.js', async: true },
   A          { src: 'reveal.js/plugin/zoom/zoom.js', async: true },
              { src: 'reveal.js/plugin/notes/notes.js', async: true }
          ],
        

@edl7878
Copy link
Contributor Author

edl7878 commented Jun 9, 2021

@edl7878 you can run tests locally using:

bundle exec rake build
bundle exec rake test

You will need to update the HTML files in https://github.com/asciidoctor/asciidoctor-reveal.js/tree/master/test/doctest

For instance, in test/doctest/custom-layout.html, you will need to fix two lines:

<script src="reveal.js/js/reveal.js"></script>

It should be <script src="reveal.js/dist/reveal.js"></script>

{ src: 'reveal.js/plugin/zoom-js/zoom.js', async: true },

It should be { src: 'reveal.js/plugin/zoom/zoom.js', async: true },

And here's the relevant test failure (taken from GitHub Action log):

(See full trace by running task with --trace)
✗  Failure: custom-layout:custom-layout
   The three different ways to hide slide titles.

          </div>
        </section>
      </div>
   E  <script src="reveal.js/js/reveal.js"></script><script>
   A  <script src="reveal.js/dist/reveal.js"></script><script>
        Array.prototype.slice.call(document.querySelectorAll('.slides section')).forEach(function(slide) {
          if (slide.getAttribute('data-background-color')) return;
          // user needs to explicitly say he wants CSS color to override otherwise we might break custom css or theme (#226)
        
          // Optional libraries used to extend on reveal.js
          dependencies: [
   E          { src: 'reveal.js/plugin/zoom-js/zoom.js', async: true },
   A          { src: 'reveal.js/plugin/zoom/zoom.js', async: true },
              { src: 'reveal.js/plugin/notes/notes.js', async: true }
          ],
        

There are 15 file issues, are these files in the test manually edited? If I modify them directly, could it be substituted by another pass of code generation?

@edl7878
Copy link
Contributor Author

edl7878 commented Jun 9, 2021

test files modified. Still has 2 errors, but I think the modifications are correct:

   E  <link href="https://cdn.jsdelivr.net/npm/[email protected]/dist/reset.css" rel="stylesheet"><link href="https://cdn.jsdelivr.net/npm/[email protected]/dist/reveal.css" rel="stylesheet"><link href="https://cdn.jsdelivr.net/npm/[email protected]/dist/theme/black.css" id="theme" rel="stylesheet">
   A  <link href="https://cdn.jsdelivr.net/npm/[email protected]/dist/reset.css" rel="stylesheet"><link href="https://cdn.jsdelivr.net/npm/[email protected]/dist/reveal.css" rel="stylesheet"><link href="https://cdn.jsdelivr.net/npm/[email protected]/dist/theme/black.css" id="theme" rel="stylesheet">
      <div class="slides">
        <section class="title" data-state="title">
          <h1>MathJax</h1>


✗  Failure: revealjs-custom-theme:revealjs-custom-theme
   please note that in this specific case it would be better to use :revealjs_theme: beige.

   E  <link href="reveal.js/dist/reset.css" rel="stylesheet"><link href="reveal.js/dist/reveal.css" rel="stylesheet"><link href="reveal.js/dist/theme/beige.css" id="theme" rel="stylesheet">
   A  <link href="reveal.js/dist/reset.css" rel="stylesheet"><link href="reveal.js/dist/reveal.css" rel="stylesheet"><link href="reveal.js/css/theme/beige.css" id="theme" rel="stylesheet">


305 examples (242 passed, 2 failed, 61 skipped)

Finished in 1.281 s.

@ggrossetie
Copy link
Member

mathjax-cdn.adoc is using a custom version of reveal.js:

:revealjsdir: https://cdn.jsdelivr.net/npm/[email protected]

So the expectation is that the specified version of reveal.js will be used.
I think we should update the value of the revealjsdir attribute to use an older version of reveal.js 4.x :revealjsdir: https://cdn.jsdelivr.net/npm/[email protected].

The same goes for revealjs-custom-theme.adoc.

Please note that reveal.js 4.1.3 was just released.

@edl7878
Copy link
Contributor Author

edl7878 commented Jun 9, 2021

Does npm has no reveal.js 4.1.1? And I will roll back several modifications? Or, 4.0.2 is preferred?

@ggrossetie
Copy link
Member

Does npm has no reveal.js 4.1.1? And I will roll back several modifications? Or, 4.0.2 is preferred?

Latest version available on npmjs is 4.1.3: https://www.npmjs.com/package/reveal.js
I was suggesting to define :revealjsdir: https://cdn.jsdelivr.net/npm/[email protected] to use a different version (for testing purpose).

Basically, we want to check that when revealjsdir is defined, the Asciidoctor reveal.js converter will use it.

@edl7878
Copy link
Contributor Author

edl7878 commented Jun 9, 2021

OK, if needed, I will target to 4.1.3.

@ggrossetie
Copy link
Member

We are almost good (according to CI), last error is:

npm can only install packages when your package.json and package-lock.json or npm-shrinkwrap.json are in sync. Please update your lock file with `npm install` before continuing.

Could you please run npm install locally and commit package-lock.json file?

@ggrossetie
Copy link
Member

One more issue catch by the tests suite:

expect(result).to.contain('<script src="node_modules/reveal.js/js/reveal.js">')

You need to update the assertion:

expect(result).to.contain('<script src="node_modules/reveal.js/dist/reveal.js">') 

@ggrossetie ggrossetie changed the title Modification for compatible with reveal.js 4.x resolves #370 add support for reveal.js 4.x Jun 13, 2021
@maxandersen
Copy link

how would i actually go about using this pr's build locally to render my own asciidoc reveal.js documents ? (i got the build working - just not sure how to refer to the local built for use with asciidcotor revealjs ?

@maxandersen
Copy link

maxandersen commented Jun 17, 2021

I figured it out.

curl -L -o m.zip https://github.com/hakimel/reveal.js/archive/master.zip

unzip it so its named reveal.js

then run bundle exec asciidoctor-revealjs test.adoc

reason im trying this is I really like the auto-animate feature in reveal.js 4.

I kinda got it working by adding: data-auto-animate=(attr 'auto-animate') in the templates but:

  1. i have to use auto-animate=true in my adoc to have it pickedup - how do i just add the attribute if present ?
  2. auto-animate relies on stable data-id attributes; any tips on how to ensure such gets applied to all kind of blocks? is it required to be manually added to every template?
  3. the animations on reveal.js are way smoother than what we get rendered - not sure if just a different default style?

@ggrossetie
Copy link
Member

i have to use auto-animate=true in my adoc to have it pickedup - how do i just add the attribute if present ?

You can use bool_data_attr method from the helper:

def bool_data_attr val
return false unless attr?(val)
if attr(val).downcase == 'false' || attr(val) == '0'
'false'
else
true
end
end

data-auto-animate=(bool_data_attr 'auto-animate')

auto-animate relies on stable data-id attributes; any tips on how to ensure such gets applied to all kind of blocks? is it required to be manually added to every template?

You can already define data attributes on virtually any blocks/elements: https://asciidoctor-revealjs-examples.netlify.app/release-4.1.html#/_data_attributes

the animations on reveal.js are way smoother than what we get rendered - not sure if just a different default style?

Not sure about this one... Apparently, it's possible to override animation settings: https://revealjs.com/auto-animate/#animation-settings

@maxandersen
Copy link

You can already define data attributes on virtually any blocks/elements: https://asciidoctor-revealjs-examples.netlify.app/release-4.1.html#/_data_attributes

cool - unfortunately seems it won't honor it on sections ? otherwise I should be able to just do:

[data-auto-animate=0]
== Test 

[source,java,data-id=code]
----
System.out.println('What upo!');
----

but here only the data-id gets through.

@maxandersen
Copy link

data-auto-animate=(bool_data_attr 'auto-animate')

I tried this but it doesn't do what I tried to ask for :) - let me try again:

With my previous fix and your suggestion above I need to write like this:

[auto-animate=true]
== Some section

[source,java,data-id="mycode"]
----
System.out.println('What upo!');
----

What I really want is:

[auto-animate]
== Some section

[source,java,data-id="mycode"]
----
System.out.println('What upo!');
----

i.e. not having to add =true as in reveal.js they just require the presence of auto-animate the value does not seem to matter.

@ggrossetie
Copy link
Member

In this case we should use an option and not an attribute. In the following example auto-animate is actually a block name:

[auto-animate]
== Some section

The reason is that we cannot disambiguate a block name from an attribute without a value. I believe that this is one of the reason why we have option:

[%auto-animate]
== Some section

cool - unfortunately seems it won't honor it on sections ? otherwise I should be able to just do:

Indeed, on <section> data attributes are explicit:

section(id=(titleless ? nil : id)
class=roles
data-transition=(attr 'transition')
data-transition-speed=(attr 'transition-speed')
data-background-color=data_background_color
data-background-image=data_background_image
data-background-size=(data_background_size || attr('background-size'))
data-background-repeat=(data_background_repeat || attr('background-repeat'))
data-background-transition=(data_background_transition || attr('background-transition'))
data-background-position=(data_background_position || attr('background-position'))
data-background-iframe=(attr "background-iframe")
data-background-video=data_background_video
data-background-video-loop=((attr? 'background-video-loop') || (option? 'loop'))
data-background-video-muted=((attr? 'background-video-muted') || (option? 'muted'))
data-background-opacity=(attr "background-opacity")
data-autoslide=(attr "autoslide")
data-state=(attr 'state'))

I would recommend the following:

data-auto-animate=((attr? 'auto-animate') || (option? 'auto-animate'))

I haven't tested it but in theory the following should work:

[%auto-animate]
== Some section
[auto-animate=]
== Some section

@edl7878
Copy link
Contributor Author

edl7878 commented Jun 17, 2021

In this case we should use an option and not an attribute. In the following example auto-animate is actually a block name:

[auto-animate]
== Some section

The reason is that we cannot disambiguate a block name from an attribute without a value. I believe that this is one of the reason why we have option:

[%auto-animate]
== Some section

cool - unfortunately seems it won't honor it on sections ? otherwise I should be able to just do:

Indeed, on <section> data attributes are explicit:

section(id=(titleless ? nil : id)
class=roles
data-transition=(attr 'transition')
data-transition-speed=(attr 'transition-speed')
data-background-color=data_background_color
data-background-image=data_background_image
data-background-size=(data_background_size || attr('background-size'))
data-background-repeat=(data_background_repeat || attr('background-repeat'))
data-background-transition=(data_background_transition || attr('background-transition'))
data-background-position=(data_background_position || attr('background-position'))
data-background-iframe=(attr "background-iframe")
data-background-video=data_background_video
data-background-video-loop=((attr? 'background-video-loop') || (option? 'loop'))
data-background-video-muted=((attr? 'background-video-muted') || (option? 'muted'))
data-background-opacity=(attr "background-opacity")
data-autoslide=(attr "autoslide")
data-state=(attr 'state'))

I would recommend the following:

data-auto-animate=((attr? 'auto-animate') || (option? 'auto-animate'))

I haven't tested it but in theory the following should work:

[%auto-animate]
== Some section
[auto-animate=]
== Some section

won't work. data-auto-animate can't map to section tag:

[data-auto-animate=true]
## test adoc1

while common block is ok, likes this:

## test adoc1

for test2

[data-auto-animate=true]
for test1

reveal.js require map to section tag:

reveal.js can automatically animate elements across slides. All you need to do is add data-auto-animate to two adjacent slide <section> elements

@edl7878 edl7878 closed this Jun 17, 2021
@edl7878
Copy link
Contributor Author

edl7878 commented Jun 17, 2021

How to reopen this pull request? It seems I closed it accidentally.

@ggrossetie ggrossetie reopened this Jun 17, 2021
@edl7878
Copy link
Contributor Author

edl7878 commented Jun 19, 2021

I think the translating layer, makes things harder for users, such as:

'fragment' if (option? :step) || (attr? 'step')
data-background-iframe=(attr "background-iframe")

why not just use reveal.js original keywords, which is easier for users.

except when use options, they have to use %, if they want cleaner output.

@edl7878
Copy link
Contributor Author

edl7878 commented Jun 19, 2021

For now, data-auto-animate is workable. test file:

# test
:revealjsdir: reveal.js

[%auto-animate]
## test adoc

[.r-fit-text]
for test1

[%auto-animate]
## test adoc1

for test2

[data-auto-animate=true]
for test1

All settings are added to section element, except data-auto-animate-delay, which is not applicable for section.

Not fully test all settings, for now.

@ggrossetie
Copy link
Member

I think the translating layer, makes things harder for users, such as:
'fragment' if (option? :step) || (attr? 'step')

I think the idea behind this abstraction is to make your content "portable" (or at least not tightly coupled to a specific backend).
As far as I remember, you can use the role fragment but it might not be supported on all elements.

[.fragment]
This is a paragraph.

data-background-iframe=(attr "background-iframe")
why not just use reveal.js original keywords, which is easier for users.

In this case we are using the same name (without the data- prefix which is an implementation detail).
I guess we could support both name, but in my opinion background-iframe should be preferred:

[data-background-iframe=https://slides.com]
== Section
[background-iframe=https://slides.com]
== Section

@edl7878
Copy link
Contributor Author

edl7878 commented Jun 19, 2021

Yes, I agree with you that after getting rid of data- prefix, it is more independent from reveal.js.

I just think that from the user's point of view, when they check the documentation of reveal.js, and find out there is a class could introduce a style, they can [.SomeStyle], and if there is a attribute to be set, they can [SomeAttribute='abc'], and if an option to be set they can [%SomeProperty].

This solution is easier for documentation in asciidoctor-reveal.js, if not, each keyword map between reveal.js to asciidoctor-reveal.js, should be demonstrated explicitly in asciidoctor-reveal.js doc.

Since this repo is asciidoctor-reveal.js, so, probably a little bit tight couple is acceptable, : )

@edl7878
Copy link
Contributor Author

edl7878 commented Jun 19, 2021

For now, the current modified code may not very optimized, any suggestions?

section.html.slim

    //reveal.js-attrib   default-value
    //data-auto-animate just set is ok
    data-auto-animate=((attr? 'auto-animate') || (option? 'auto-animate'))
    //data-auto-animate-easing	ease
    data-auto-animate-easing=((attr? 'auto-animate-easing') || (option? 'auto-animate-easing'))
    //data-auto-animate-unmatched	true
    data-auto-animate-unmatched=((attr? 'auto-animate-unmatched') || (option? 'auto-animate-unmatched'))
    //data-auto-animate-duration	1.0
    data-auto-animate-duration=((attr? 'auto-animate-duration') || (option? 'auto-animate-duration'))
    //data-auto-animate-id	absent user need to set the value
    data-auto-animate-id=(attr 'auto-animate-duration')
   //data-auto-animate-restart	absent just set is ok
    data-auto-animate-restart=((attr? 'auto-animate-restart') || (option? 'auto-animate-restart')))

@ggrossetie
Copy link
Member

For now, the current modified code may not very optimized, any suggestions?

If it helps reducing code duplication, you can add an helper method in https://github.com/asciidoctor/asciidoctor-reveal.js/blob/master/templates/helpers.rb

@edl7878
Copy link
Contributor Author

edl7878 commented Jun 19, 2021

May I ask, how to export an windows standlone exe converter?

@mojavelinux
Copy link
Member

The conversation about attribute and option mapping strikes me as very off topic for this issue. I think it should be moved to a separate issue so it doesn't distract from the work being done here.

@ggrossetie
Copy link
Member

The conversation about attribute and option mapping strikes me as very off topic for this issue. I think it should be moved to a separate issue so it doesn't distract from the work being done here.

I agree we should focus on supporting reveal.js 4.x. We can create follow-up issues/pull requests to improve attributes and options mapping.

May I ask, how to export an windows standlone exe converter?

You need to execute npm run package. Please note that you first need to install Node and install dependencies (using npm i).

I just noticed that one test is failing due to one of my change, I will fix it.

@ggrossetie
Copy link
Member

@edl7878 I will also rebase, squash commits and force push.

For the next time, please create a dedicated branch using the following convention: issue-{{issue-number}}-{{short-description}} (in this case, it could have been: issue-370-support-reveal-js-4-x). Also, please use rebase (and not merge) to synchronize with the latest changes on the main branch. It makes it easier to rewrite history and squash commits.

data-auto-animate-easing=((attr? 'auto-animate-easing') || (option? 'auto-animate-easing'))
data-auto-animate-unmatched=((attr? 'auto-animate-unmatched') || (option? 'auto-animate-unmatched'))
data-auto-animate-duration=((attr? 'auto-animate-duration') || (option? 'auto-animate-duration'))
data-auto-animate-id=(attr 'auto-animate-duration')
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
data-auto-animate-id=(attr 'auto-animate-duration')
data-auto-animate-id=(attr 'auto-animate-id')

@@ -63,7 +63,13 @@
data-background-video-muted=((attr? 'background-video-muted') || (option? 'muted'))
data-background-opacity=(attr "background-opacity")
data-autoslide=(attr "autoslide")
data-state=(attr 'state'))
data-state=(attr 'state')
data-auto-animate=((attr? 'auto-animate') || (option? 'auto-animate'))
Copy link
Member

Choose a reason for hiding this comment

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

We need to add a sample presentation in the examples directory to demonstrate how to use these attributes/options.

It serves multiple purposes:

  • we can manually test that everything is working fine by opening the generated presentation in a browser
  • we can write tests (using the existing test infrastructure)
  • we are "self-documenting" new features

I think we should also add a new page in the documentation named auto-animate.adoc to describe this new feature in the context of an AsciiDoc presentation.

@edl7878
Copy link
Contributor Author

edl7878 commented Jun 19, 2021

@edl7878 I will also rebase, squash commits and force push.

For the next time, please create a dedicated branch using the following convention: issue-{{issue-number}}-{{short-description}} (in this case, it could have been: issue-370-support-reveal-js-4-x). Also, please use rebase (and not merge) to synchronize with the latest changes on the main branch. It makes it easier to rewrite history and squash commits.

OK. Got it.

@maxandersen
Copy link

Awesome!

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.

4 participants