-
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
Config plugins #196
Config plugins #196
Conversation
no idea why or what on travis if failing. changes are working great here. please help! |
Thanks for your contribution! Sorry I haven't replied earlier. A quick glance and everything looks fine. Plus, I like the design. I'm going to test it later and report back. Travis failures are related to the fact that your code changed the output of the generated HTML. We do strict regression testing against these. Don't worry about it, I can fix that. One thing I'm wondering: I'm not sure attributes is a list, I think it's a hashmap (key / value) so multiple |
Thanks for your feedback! example from what I use: cat plugins_add.js
cat plugins_setup.js
if the concept of this solution will be approved I can improve the documentation and put up a sample knowing that it is just about some (maybe withe spaces) in a test seems fixable, but I am on holidays this week, can have a look at it in 2 weeks if you have not time |
templates/document.html.slim
Outdated
@@ -21,7 +21,7 @@ html lang=(attr :lang, 'en' unless attr? :nolang) | |||
link rel='stylesheet' href='#{revealjsdir}/css/theme/#{attr 'revealjs_theme', 'black'}.css' id='theme' | |||
- if attr? :icons, 'font' | |||
- if attr? 'iconfont-remote' | |||
link rel='stylesheet' href=(attr 'iconfont-cdn', %(#{cdn_base}/font-awesome/4.3.0/css/font-awesome.min.css)) | |||
link rel='stylesheet' href=(attr 'iconfont-cdn', %(#{cdn_base}/font-awesome/4.5.0/css/font-awesome.min.css)) |
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.
This should be done in a separate PR
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.
agree, there is even a newer font-awesome
Gotcha. Then I think the name I did test one and two plugins tonight and it works well. Good job on finding and implementing an elegant design! 👍 It also is almost like the When I have time, I'll merge it, fix the tests, make the changes I've mentioned, clarify the documentation a little bit and add the example I just did to the repo. My test involved the chalkboard plugin and the menu plugin. |
I am glad you like the solution! |
print-pdf is a script for phantomjs it is not meant to be included in a rendered presentation. It causes an error to the console.
This reverts commit 8a75f49.
Made the discussed changes. Feel free to comment. Will merge if no objections. Thanks for your contribution @a4z! |
Glad you like it! |
I use speaker notes extensively and I never noticed. I never used reveal.js hosted by a CDN because I don't trust conference networks 😉. It's probably blocked because of a same origin policy violation. I know speaker notes relies on PostMessage to pass state around. Are you sure hosting only the notes plugin locally fixes the issue? Also, if you want reveal.js hosted on a CDN, isn't hosting plugins locally defeating the purpose? I think the issues you raise are interesting but I would file a separate issue for them and merge this PR. What do you think? |
I use the CDN especially during creation of presentation but also for talks company intern since I switch computers very often, and this makes things easy. so far I never encountered any problem, but for important talks I have everything as a bundle. I am thinking about striping out the whole plugin configuration section into a file, and load it, since I have always a config file. And most of the default loaded plugins are not used by me so I see no point in loading them, while I add other. but the first thought is, why not strip out the whole section if there is something custom give, otherwise use the default .... PS: and without writing a draft, I do not know how compatible this would be with the solution in this PR |
Thanks for your effort and improvements @obilodeau !! |
this is a suggestion to solve #118 through allowing to specify optional files that can be used to add and configure plugins.
Please see changes in README.adoc for more information