-
Notifications
You must be signed in to change notification settings - Fork 188
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
Adds package.json #95
Conversation
This is good. In the future, though, I'd really like to set a goal that the converter can be loaded through the backend option instead of referencing the templates directly. That's a matter of creating a Ruby or JavaScript-based converter class that sets up the template (and/or composite) converter and points it at the templates. |
Since I'm using a custom template converter I can add a default value when running on Node: if backend == 'revealjs' && running_node
template_dir = template_dir || 'node_modules/asciidoctor-reveal.js/templates'
end @mojavelinux Is this what you have in mind ? |
That's a good interim step. I'd still like to see a custom converter introduced to manage the wiring because it can do other things such as set the basebackend and htmlsyntax attributes. In other words, official converters should always be registered formally with the API. Using templates "ad-hoc" is only meant to be an end-user customization. |
I've just noticed that @obilodeau is working on an "official" converter registered with the API. But AFAIK this won't work in a JavaScript environment. Currently there's multiple issues with template converter backend in JavaScript:
But I still think that the template converter backend is great for end user customization. However "official" converters should be easier to use/integrate. Maybe we should rewrite this converter in Ruby like the html5 converter ? But that's another subject :) |
I like the by default value that you suggest |
For me this is a complex change to handle since I don't fully understand all of the stuff that is going on. That said, I'm willing to integrate anything that makes using AsciiDoc (including asciidoc[tor] + reveal.js) simpler as long as someone is around when issues arises. However there are two things I want to say here: First, you need to add some documentation about it in the README. Even if I would like to test what you propose I have no idea how to do so. Some doc would help me (and others) for sure. Second is that I'll let @mojavelinux give the final 👍 on this one. Since he's the one who understands all the moving pieces the most. Since reveal.js is a node project I think this can't be a bad thing to have this back-end published over there as well. Thanks for that! |
Sure, that's fair enough, I will be around 😉
I didn't know how to integrate with the current README, I think we will need to restructure the sections a bit. Currently the "Prerequisites" and "Rendering the AsciiDoc into slides" sections explain how to use Reveal.js templates with Asciidoctor Ruby.
Ok
👍 |
Yes, do some intro like: there are two ways to use this back-end with varying levels of functionality. Then put all the ruby specific in a section and the javascript specific in another. Don't worry too much about phrasing, we can fix that later. As long as the steps are there for me to try the back-end. |
ab41e73
to
db6e993
Compare
I found an issue, when using a Node package To workaround this issue we can simply create a link: Instead I think we should add a global variable to set the "reveal.js base directory": https://github.com/asciidoctor/asciidoctor-reveal.js/blob/master/templates/jade/document.jade - link(href="reveal.js/css/reveal.css",rel="stylesheet")
+ link(href="#{revealjs_base_dir}/css/reveal.css",rel="stylesheet") |
I think that this is a good idea to setup this |
Can't you just tell the user how to configure the existing Most people I know who use revealjs fork it to add a custom theme. Using On Mon, Sep 19, 2016, 11:34 AM Guillaume Grossetie [email protected]
|
Sure but |
It's not up to the template engine. AsciiDoc document attributes are available in both engines as it is provided by the core. It's just that the Jade template designer chose not to use it (for a reason he didn't explain). Task added to #63.
Please absolutely refrain from introducing new revealjs specific attributes if they already exist for slim. This will only make multi-engine maintenance more complicated. |
I didn't know about
When I said "only defined", I meant "only used" in Slim templates.
I think it's not on purpose since I don't see any real drawback... ? |
Agreed. Not knowing it existed is a perfectly valid reason not to use it. I On Mon, Sep 19, 2016, 5:34 PM Guillaume Grossetie [email protected]
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not able to test since the packages are not in the npm registry. See inline comments. I suggest you provide instructions on how to test the whole thing in HACKING.adoc
.
For me what is important as an outsider to the javascript/node ecosystem is what I can independently reproduce with your instructions (and with the least effort).
Thanks for taking the time to educate me and others! Improvements to the documentation will increase the number of users and ease the path to contributors.
|
||
Using npm: | ||
|
||
$ npm i -g npm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disclaimer: I'm a node/javascript n00b.
Why the global install of npm (with npm!?)? On my arch linux setup npm is managed by my package manager and I don't want to change that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npm is the toolset required to manage on your machine the nodes modules/packages. This toolbox is installed globally along your node distribution. The goal of this command is to be sure that npm is up to date : https://docs.npmjs.com/getting-started/installing-node#updating-npm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took inspiration from https://github.com/lodash/lodash#installation, the only hard requirement is to have npm available on your PATH. So basically if you install Node.js you are good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest you simply add a Prerequisite section with Node.js then. Link it to the website. This way people will figure out how to install it on their own based on their requirement, preference, system, knowledge, etc.
|
||
$ npm i -g npm | ||
$ npm i --save asciidoctor.js | ||
$ npm i --save asciidoctor-reveal.js |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
npm ERR! 404 Registry returned 404 for GET on https://registry.npmjs.org/asciidoctor-reveal.js
So how can I test locally?
I tried using npm link
, even as root (which I hate doing) and it always failed because of a missing dependency on asciidoctor-template.js
:
npm ERR! 404 'asciidoctor-template.js' is not in the npm registry.
So I tried doing a sudo npm link
inside a checkout of asciidoctor-template.js and it worked but a subsequent sudo npm link
from asciidoctor-reveal.js still failed like if asciidoctor-template.js was not installed:
npm ERR! 404 'asciidoctor-template.js' is not in the npm registry.
Sorry for my lack of experience but I tried. Can you help me here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The module asciidoctor-reveal.js hasn't been yet published on npmjs.com server (https://www.npmjs.com/search?q=asciidoctor-reveal.js). This is why you an error 404.
If you use this command npm link /path
, you can copy the package from this project cloned locally on your machine https://github.com/Mogztter/asciidoctor-reveal.js (use node-package branch please) to the folder where you would like to test.
Remark: As the project asciidoctor-template hasn't been also published on npmjs server, then you must clone & copy the path to install the modules
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that, I will publish a "beta" version in the npm registry, to make things easier.
$ npm i --save asciidoctor.js | ||
$ npm i --save asciidoctor-reveal.js | ||
|
||
In Node.js: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You provide the filename and the content, which is good but then how do you invoke the actual rendering? It is unclear to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What Guillaume wants to say here is that : "Create a node.js file containing the required info to configure Asciidoctor (Opal, modules, Attributes, ...) and next you can run it on your machine using the command node node.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following line is calling the Asciidoctor API to render the document: Asciidoctor.$convert_file('presentation.adoc', options);
I you want to provide the content, you need to use Asciidoctor.$convert(content, options);
var options = Opal.hash({safe: 'safe', | ||
template_dir: 'node_modules/asciidoctor-reveal.js/templates', | ||
backend: 'revealjs'}); | ||
var result = Asciidoctor.$convert_file('presentation.adoc', options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact that presentation.adoc
is a filename and that it is related to what you are trying to render is important and should be clarified in the documentation outside the code listing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
Here are the bash instructions that you can use to create and test a node project using Guillaume's modifications. Instructions : Create a bash file, change the permissions (chmod +x file) & execute it within a folder
|
Wow thanks @cmoulliard 😄 @obilodeau I will try to make the requested changes ASAP |
Thanks! For reference, I needed to do another
After this the render worked:
Note that I needed to work around the empty The render is missing a title page and has other problems but those were expected and does not prevent us from merging this. |
The bash script should be turned into instructions and added into |
What do you mean by "in a one-liner" ? Basically, The template converter role is:
If you are wondering, lib/asciidoctor/core_ext/factory.rb is a monkey-patch of lib/asciidoctor/converter/factory.rb to remove the Does that make sense ? 😀 💭 |
Once the README is fixed up and test setup instructions are written in |
My $ git diff
diff --git a/package.json b/package.json
index b109129..294cb0d 100644
--- a/package.json
+++ b/package.json
@@ -34,7 +34,8 @@
},
"homepage": "https://github.com/asciidoctor/asciidoctor-reveal.js",
"dependencies": {
- "reveal.js": "3.3.0",
- "asciidoctor-template.js": "1.5.5-2"
+ "asciidoctor-template.js": "1.5.5-2",
+ "asciidoctor.js": "^1.5.5-2",
+ "reveal.js": "3.3.0"
}
} I don't know if this is expected or not? |
53bccfc
to
1de3de2
Compare
@obilodeau Done, let me know if this is OK I've added a section on I've also fixed this issue #88 and used |
1a99d8f
to
e085136
Compare
Done!
Confirmed, this is related to the I'm currently running version |
I tend to disagree. Running this as root overwrites files that should be under the authority of my package manager. This makes package integrity verification impossible which I don't accept with from a security standpoint. |
var attributes = 'revealjsdir=node_modules/asciidoctor-reveal.js/node_modules/reveal.js'; | ||
var options = Opal.hash({safe: 'safe', | ||
backend: 'revealjs', | ||
attributes: attributes}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing attributes as arguments makes overriding them in the document impossible. See the documentation on attribute assignment precedence for more information about that.
I suggest that you push the revealjsdir attribute default value in the example document as I suggested earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I don't think that it's a good practice to define "rendering attribute" in the document. revealjsdir
only make sense if my backend is revealjs
.
My document could be rendered as an HTML5 presentation with Reveal.js or Bespoke.js or even as a PDF...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that a presentation could be targeting several back-end but in 10+ presentations I did in asciidoc there was always something back-end specific. On the other hand, the reveal.js package location issue is real. Yours is not at the same location than mine. What do you propose as a solution to satisfy both location that don't require crippling OS packages? Maybe a check in the javascript shim? I would accept that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just add a trailing @ to the value and the attribute can be overridden by the document. Problem solved ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just add a trailing @ to the value and the attribute can be overridden by the document.
And I was the one to refer to the documentation... 🤦
I'm opening up a PR with just that.
HACKING and README are too similar and HACKING is actually more complete. I'm merging this and I'll fix the redundancy by moving HACKING's content into README and pushing Thanks for the work! |
* Removed redundancy between HACKING and README doc (DRY principle) * Respected attribute precedence on :revealjsdir:
Ok thanks 😄 |
I don't. Is it easy to take over the maintenance of a package? If so, I would suggest that you publish it under your name since you proposed to be the maintainer of the javascript side of this back-end. |
@obilodeau Ok, I don't remember how the "ownership" of a Node package works but I will try 👍 |
@mojavelinux Done! https://github.com/asciidoctor/asciidoctor-template.js |
I was using Node 4.5, so maybe it was a bug in that version. Either way, I think Node 4 should be avoided since it was the first version after the big merge. I can see still supporting 0.12.0 since a lot of people were stuck on it, but even the last version of Fedora is on Node 6 now. Regardless, it's just something to be aware if people do report it. |
Aha. That makes sense. |
I think we should simply say "If your node_modules does not have a flat hierarchy, then please update your version of npm. That's an indication that the version of npm on your machine is outdated." |
👍 Permission updated! |
Ok found something. Just add me and @mojavelinux as owner on the package https://docs.npmjs.com/cli/owner If you want to go this way let me know and I'll create an account. On Wed, Sep 28, 2016, 6:16 PM Guillaume Grossetie [email protected]
|
👍 That's exactly the arrangement I have on other packages I help maintain. In my opinion, npmjs.com has a better ownership model than other repositories for this reason. |
Related to discussion in asciidoctor#95. Allows our example to avoid :revealjsdir: document attribute in recent node versions while still allowing per-document overrides. Took the opportunity to reverse the default meaning as discussed.
My [new] npmjs.com username is obilodeau. |
@obilodeau @mojavelinux Package published! You are both collaborators 🎉 |
Great! Are you planning to do an announcement on discuss.asciidoctor.org? I think downstream would benefit, namely, the AsciidocFX people (@rahmanusta) and adoc-editor (@mgreau) would benefit from this work. And there's the chrome extensions, etc. Getting the word out would help.
I googled a bit and didn't find an easy way out... I hoped that we could override the README.adoc with some field in |
Yes I will do an announcement but first I need to fix a major issue in the I also want to improve the documentation and test the integration myself Le 30 sept. 2016 5:49 AM, "Olivier Bilodeau" [email protected] a
|
Understood. Please note that I modified the structure of the documentation We also created a starter repository with bundler instructions. We might https://github.com/obilodeau/presentation-revealjs-starter On Fri, Sep 30, 2016, 3:20 AM Guillaume Grossetie [email protected]
|
Yes, that is possible. That's what we do for the bespoke plugins. You can see an example here: https://github.com/opendevise/bespoke-multimedia. What I do is transform the AsciiDoc to Markdown (abbreviated) and add it the readme field in package.json. That's one approach. The other approach is to use the Markdown compatibility support in Asciidoctor to get a reasonable interpretation of the AsciiDoc. See http://asciidoctor.org/docs/asciidoc-syntax-quick-reference/#markdown-compatibility. Ultimately, we need to get npm to support AsciiDoc. Frankly, it's ridiculous that they don't. |
PR incoming!!
I filed an issue here: npm/npm#14159. They use this parser for README's: https://www.npmjs.com/package/marky-markdown I feel like this won't get fixed since it looks like a deep architectural problem (enabling mutliple README processing engine) and npm is clearly winning right now in terms of {mind,market}share. Maybe I'm wrong but the readme field interim is good enough for most projects to at least point to GitHub without having to sacrifice their own markup choice. That said, I did the right thing ™️ by opening an issue so I feel good about that 😉 |
The goal is to make this backend easily available on the Node ecosystem.
I've created a simple converter for Jade templates (compiled in JavaScript with Opal) that will be available soon on
npmjs
: https://github.com/Mogztter/asciidoctor-template.jsWhen both projets will be published on
npmjs
, a user will be able to convert an AsciiDoc document into a Reveal.js presentation as follows:npm install asciidoctor.js # AFAIK this is only required on npm 2.x npm install asciidoctor-reveal.js
Feel free to add authors and a better description 😄