search

asciidoctor / asciidoctor-extensions-lab

A lab for testing and demonstrating Asciidoctor extensions. Please do not use this code in production. If you want to use one of these extensions in your application, create a new project, import the code, and distribute it as a RubyGem. You can then request to make it a top-level project under the Asciidoctor organization.
Other
104 stars 101 forks source link

Add Chartist.js backend to the chart macro extension #32

Closed ggrossetie closed 9 years ago

ggrossetie commented 9 years ago

Resolves #24

mojavelinux commented 9 years ago

Let me know when you are ready for me to review / merge.

ggrossetie commented 9 years ago

c3js + Opal is not working great, I will push forward Chartist.js

ggrossetie commented 9 years ago

I made some changes to be able to use this extension in the Chrome extension

And... :tada:

optimised

mojavelinux commented 9 years ago

Wow! That's incredible! I'm checking it out now.

:tada: on!

mojavelinux commented 9 years ago

Aside from the inline comments I made, I think this looks good to merge!

I'm not sure whether engine should be a positional attribute, or at least whether it should be the second position if it is. What about the dimensions of the chart? I wonder if we should think about those as positional attributes.

chart::data.csv[line,600,400,engine=chartist]

Maybe the engine should be a document-level attribute.

chart-engine: chartist

That seems to make more sense as you probably are going to use a single engine for all your charts in most cases. Then, the explicit, non-positional attribute override per block makes more sense.

We could even consider a shorthand like:

chart::data.csv[line@chartist,600,400]

wdyt?

mojavelinux commented 9 years ago

The Chrome extension works nicely. Only one problem is that if you don't set the chart engine, Asciidoctor.js crashes. Being able to set chart-engine at the document level should help resolve that issue (as we can set it in the default attributes).

ggrossetie commented 9 years ago

chart::data.csv[line,600,400,engine=chartist]

I like it, that's more consistent and looks very similar to the image block/inline macro.

The Chrome extension works nicely. Only one problem is that if you don't set the chart engine, Asciidoctor.js crashes. Being able to set chart-engine at the document level should help resolve that issue (as we can set it in the default attributes).

Yes, currently the Chrome extension only works with Chartist engine. I will force the :chart-engine: chartist at the document level.

When using normalize_asset_path and read_asset I get the following exception in safe mode

SecurityError : asciidoctor: FAILED: : Failed to parse source, Jail is not an absolute path: file:///workspace/asciidoctor-extensions-lab/lib/chart-block-macro

And this one with unsafe

Exception : asciidoctor: FAILED: : Failed to parse source, (intermediate value)(intermediate value)(intermediate value).$readable? is not a function
mojavelinux commented 9 years ago

When using normalize_asset_path and read_asset I get the following exception in safe mode

Hmm. Let's defer that an open a separate issue. I need to explore more, but I don't want to hold things up. What you have now has the right path addition, so we are starting with the right foot forward.

mojavelinux commented 9 years ago

that's more consistent and looks very similar to the image block/inline macro.

Indeed. Consistency is a key strength for us.

ggrossetie commented 9 years ago

Hmm. Let's defer that an open a separate issue. I need to explore more, but I don't want to hold things up. What you have now has the right path addition, so we are starting with the right foot forward.

Ok, can I merge the pull request ?

mojavelinux commented 9 years ago

Absolutely! This looks superb!