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

Config plugins #196

Merged
merged 9 commits into from
Jul 28, 2018
Merged

Config plugins #196

merged 9 commits into from
Jul 28, 2018

Conversation

a4z
Copy link
Contributor

@a4z a4z commented Jun 10, 2018

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

@a4z
Copy link
Contributor Author

a4z commented Jun 10, 2018

no idea why or what on travis if failing. changes are working great here. please help!

@obilodeau
Copy link
Member

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 revealjs_plugins_add statements will overwrite each other. I'll verify that by trying to add two plugins.

@a4z
Copy link
Contributor Author

a4z commented Jun 24, 2018

Thanks for your feedback!
multiple revealjs_plugins_add makes no sense, the file file given needs to have a comma separated list of file path to plugins

example from what I use:

cat plugins_add.js

{ src: 'config/plugins/mouse-pointer.js' },
{ src: 'config/plugins/badges.js' }

cat plugins_setup.js

       badges: {
          languages: true
        }

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

@@ -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))
Copy link
Member

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

Copy link
Contributor Author

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

@obilodeau
Copy link
Member

obilodeau commented Jun 25, 2018

multiple revealjs_plugins_add makes no sense, the file file given needs to have a comma separated list of file path to plugins

Gotcha. Then I think the name revealjs_plugins_add should just be revealjs_plugins and revealjs_plugins_setup be revealjs_plugins_configuration as this is more aligned with reveal.js' language. Is this fine for you?

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 customcss attribute we added.

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.

@a4z
Copy link
Contributor Author

a4z commented Jun 25, 2018

I am glad you like the solution!
Thanks for you effort to fix the open points and your work to pull this in!

@obilodeau
Copy link
Member

Made the discussed changes. Feel free to comment. Will merge if no objections.

Thanks for your contribution @a4z!

@a4z
Copy link
Contributor Author

a4z commented Jul 23, 2018

Glad you like it!
The only thing I noticed since last time, and where I started to think about how to solve it, is that the notes plugin (where you see the next slide in a new window) does not work via revealjs over CDN.
In this case the node plugin needs to be added manual to a custom folder.
So maybe it should be removed from the default loaded plugins.
Also, the loaded markdown plugins may not be used ever, and loading them should be avoided.
So I start to think if there should be any plugin loaded per default....

@obilodeau
Copy link
Member

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?

@a4z
Copy link
Contributor Author

a4z commented Jul 23, 2018

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.
yes, copying the plugin to a user defined folder. like custom plugins, and load it from there work with revealjs via CDN. Just not load it twice, with the default path, and a custom one.
That's why I had to change the default template, and when I was there I also removed the plugins I do not need because things load slightly faster if this is omitted.

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.
I fixed this 10 minute before a talk in the company I am working, and did not have time to thing about since then.
And currently I am on vacation, so I think I will not have something this week,

but the first thought is, why not strip out the whole section if there is something custom give, otherwise use the default ....
it should theoretical be possible to read an other file if no file is given, so ...
will try it after vacation

PS: and without writing a draft, I do not know how compatible this would be with the solution in this PR

@obilodeau obilodeau merged commit 97df11c into asciidoctor:master Jul 28, 2018
@a4z
Copy link
Contributor Author

a4z commented Jul 28, 2018

Thanks for your effort and improvements @obilodeau !!

@obilodeau obilodeau added this to the 1.2.0 milestone Oct 16, 2018
@a4z a4z deleted the config-plugins branch August 18, 2020 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants