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

Reveal.js 3.8.0 to 3.9.2 support #301

Merged
merged 31 commits into from
Feb 8, 2020

Conversation

obilodeau
Copy link
Member

@obilodeau obilodeau commented Dec 31, 2019

Draft commit for reveal.js 3.8.0 support.

This work is postponed to have a very stable 3.0.0 that will support reveal.js versions spanning from 3.0 to 3.7. See #296.

Needs work:

  • syntax highlight <mark> syntax described here does not work
  • line number highlights described here do not work
  • default plugin call for highlightjs is not current but it doesn't seem to break anything
  • I'm not sure per-slide iframe preloading options are supported
  • Also, see commits messages and introduced comments for more to add to the task list
  • test hash feature w/ and w/o history and see if direct links still works

@obilodeau
Copy link
Member Author

  • Rebased on top of 3.1.0
  • Aligned with latest reveal.js index.html template for slide decks (dropped IE hacks, safari viewport stuff, etc.)
  • Highlightjs changes (the way its called, default css to align with upstream)
  • Dropped redundant print-pdf plugin call

Also, highlightjs <mark> does work if we put valid html angle brackets in the generated source file so we need to tell Asciidoctor to let them through. Result is ugly though but it seems to be intended (I tested with only reveal.js).

@obilodeau obilodeau changed the title Reveal.js 3.8.0 support Reveal.js 3.8.0 and 3.9.1 support Jan 29, 2020
@obilodeau
Copy link
Member Author

Tonight's work:

  • highlightjs <mark> works with subs="none", adjusted the example accordingly
  • highlightjs line highlight works, it kicks-in line highlighting which might be unexpected and there seems to be a CSS activated that breaks the output. Still need to work on this.

highlightjs-highlight-ugly

  • added broken examples for other supported highlighters, given I have little time to spend on this project (and I'm spending a lot right now) I won't focus on them but they are documented as broken and people can step in

@ggrossetie
Copy link
Member

@obilodeau How can I help with this pull request? If you have specific tasks, please tell me. Hopefully I've fixed the docinfo issue in #324

@obilodeau
Copy link
Member Author

Rebased on top of #324.

@obilodeau
Copy link
Member Author

Highlight.js is feature parity with 3.8.0. I'm going to deal with 3.8.0 to 3.9.1 3.9.2 later.

Next-up: go through all configuration options and see if I'm missing anything

@obilodeau
Copy link
Member Author

Pushed doctests

@obilodeau
Copy link
Member Author

Lazy loading is not supported at the moment and would require a lot of testing and changes. I have video-heavy presentations and never felt it was required. Also, it can be implemented later without causing a breaking change. Let's not implement it now.

@obilodeau obilodeau changed the title Reveal.js 3.8.0 and 3.9.1 support Reveal.js 3.8.0 to 3.9.2 support Feb 5, 2020
@obilodeau obilodeau marked this pull request as ready for review February 5, 2020 05:17
@obilodeau
Copy link
Member Author

This is ready to go in. I have a big presentation coming up so I would like to release this relatively shortly (in about a week max).

@Mogztter if you want #320 to get in for 4.0.0, I would advise you to get it in shape quickly. I would prefer it if it does get in. Plus there is a lot of doctest around highlightjs so I feel confident.

@mojavelinux there's an API question that you might want to look at in this review

Lastly, @Mogztter since you asked how you can help, there's #326 that I would like to get done soon. I'm not very good at maintainable CSS so if you can give it a try I would appreciate it.

@ggrossetie
Copy link
Member

ggrossetie commented Feb 5, 2020

Speaking of #326, I need to do some vertical alignment and as far as I know the only way to do it reliably is to wrap the content of a slide in a <div>. More specifically I want to keep the title on top left but center the content.
Is that something you would consider or should I override the "section" template and do it in my presentation?

@obilodeau
Copy link
Member Author

obilodeau commented Feb 5, 2020 via email

@ggrossetie
Copy link
Member

I don't want to pollute the pull request about the reveal.js upgrade, so here's my reply: #326 (comment)

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.

LGTM, good job 👍
I left some nitpicks but otherwise 🚀

@ggrossetie
Copy link
Member

if you want #320 to get in for 4.0.0, I would advise you to get it in shape quickly. I would prefer it if it does get in. Plus there is a lot of doctest around highlightjs so I feel confident.

Alright, I will work on it tomorrow 😉

@obilodeau
Copy link
Member Author

Fixed whitespace

…stitution

Also wrapped up support for step-by-step highlights on top
and updated tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants