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

Add data-noescape to highlightjs callouts #167

Merged
merged 7 commits into from
Dec 27, 2017

Conversation

dschulten
Copy link
Contributor

Closes #166

@obilodeau
Copy link
Member

Made changes to avoid doctest failures. Will merge if tests all clear. Thanks!

@dschulten
Copy link
Contributor Author

dschulten commented Dec 27, 2017

Hm, what I intended was that the data-noescape attribute should not appear if code_noescape = false. I need to have another look. There might be an issue with Slim, it should not render false or nil attributes as per literate tests, but for some reason we get data-noescape=""

@obilodeau
Copy link
Member

obilodeau commented Dec 27, 2017

Per the tests you referred, we should have the intended behavior. That said, I think the converter is not set in html5 mode. That's why we get data-noespcape="" but in HTML this has the same effect on the DOM.

@obilodeau
Copy link
Member

I just manually confirmed that with code_noescape = false the data-noescape doesn't appear:

<div class="listingblock"><div class="content"><pre class="highlight"><code>fn main() {
    println!("Hello World!");
}</code></pre></div></div>

@obilodeau obilodeau merged commit 0ef7b0c into asciidoctor:master Dec 27, 2017
@dschulten dschulten deleted the patch-2 branch December 28, 2017 06:17
@dschulten
Copy link
Contributor Author

Also html source code will still be escaped with this. It looks we're fine :)

@mojavelinux
Copy link
Member

Is this something we should add to core as well? highlight.js can really be configured to ignore the callout bubbles? This is something for which we've long been seeking a solution.

@obilodeau
Copy link
Member

@mojavelinux: This seems to be a reveal.js feature not a highlightjs one: https://github.com/hakimel/reveal.js/blob/a0c013606e130baad976195730dcd7e7cd9e2855/plugin/highlight/highlight.js#L62

However you might want to reimplement that inside your html5 converter.

@obilodeau
Copy link
Member

There is something I haven't thought of before that will be broken with this change: when the code in the listing is html... I'll open a new ticket about that.

@obilodeau
Copy link
Member

It works out ok after all.

@mojavelinux
Copy link
Member

I see now. It was actually reveal.js that was causing the problem. All we want is for reveal not to mess with the block since it was working before reveal.js started messing with it.

And it turns out, highlight.js doesn't have problems with callouts in the HTML (it seems to understand how to skip them), so we're actually good.

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