ggrossetie / asciidoctor-web-pdf

Convert AsciiDoc documents to PDF using web technologies
https://asciidoctor.org
MIT License
449 stars 92 forks source link

Roles should be space-separated in templates #21

Closed matklad closed 5 years ago

matklad commented 5 years ago

Currently, templates use

class="${node.getRoles()}"

This, however, joins roles via ,, which is not what we want. It should be this instead:

class="${node.getRoles().join(" ")}"

Is there some helper method we could use here instead of writing .join every time?

ggrossetie commented 5 years ago

Hello @matklad.

Indeed it should be node.getRoles().join(' ').

Is there some helper method we could use here instead of writing .join every time?

If we do introduce an helper method, I think this function should be defined in the converter (not in Asciidoctor.js) because it's really a converter (and more specifically an HTML converter concern) where roles are used as class attribute value (and thus should be joined with a space).

matklad commented 5 years ago

Indeed! I've took a quick look at the convertor (html5.rb file), and it mostly uses ["some-hard-coded-class" node.role].compact pattern, so a helper method woudn't be useful for it directly.

I think we can add somehting like node.getClass() which returns a space separated string. On the one hand, this looks like a quirky API: it's better to deal with lists and convert to string as the last step. On the other hand, because in -pdf.js we use string-based templates, a string is exactly what we need.

Perhaps the best solution is to punt on this, for now, and just write .join(' ') by hand?

ggrossetie commented 5 years ago

Perhaps the best solution is to punt on this, for now, and just write .join(' ') by hand?

That's my opinion and if it gets out of hand we could reconsider and provide an helper method based on the usage.