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

Defined an idprefix to avoid reveal.js bug #28

Merged
merged 1 commit into from
Aug 12, 2016
Merged

Defined an idprefix to avoid reveal.js bug #28

merged 1 commit into from
Aug 12, 2016

Conversation

schauder
Copy link
Contributor

@schauder schauder commented Aug 1, 2016

There is a bug in reveal.js that causes problems, when ids start with non-ascii-characters, although it is perfectly legal html5. Setting the idprefix avoids this problem and I think therefore this should be used as the default in the example.

See also:
hakimel/reveal.js#1346
hakimel/reveal.js#1230
asciidoctor/asciidoctor-reveal.js#32

There is a bug in reveal.js that causes problems, when ids start with non-ascii-characters, although it is perfectly legal html5. Setting the ``idprefix`` avoids this problem and I think therefore this should be used as the default in the example.
@mojavelinux
Copy link
Member

I support this change. Using a blank idprefix is risk for a few reasons. In addition to getting IDs that start with non-ASCII characters (legal HTML5, but not supported by reveal.js), you also run the risk of getting an ID that conflicts with built-in IDs on the page / template (e.g., header/footer). Generally, it's a good idea to use a non-blank idprefix.

Note that the default idprefix is underscore for this very reason.

@rwinch rwinch self-assigned this Aug 12, 2016
@rwinch rwinch merged commit 620acf2 into asciidoctor:master Aug 12, 2016
@rwinch
Copy link
Member

rwinch commented Aug 12, 2016

Thank you for your PR @schauder! This is now merged into master

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