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

Optimize rendering #33

Closed
wants to merge 34 commits into from
Closed

Conversation

jceb
Copy link
Contributor

@jceb jceb commented Sep 29, 2019

(This includes the theming branch)

I converted the rendering loop to a Makefile so that unnecessary file operations are avoided. In addition, I moved the temporary files to the .meta/ and the rendered folder to assets/rendered.

@@ -2,7 +2,7 @@
set -e -E -u

SOURCES="/source/slide*.md $(find /source/assets -type d) /source/include-before-body /source/include-after-body"
TARGET=/target/index.html
TARGET="$(mktemp)"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not /target/index.html?
With index.html as target, you also detect changes that happened before wait-for-changes was called.

@jceb
Copy link
Contributor Author

jceb commented Oct 1, 2019

The reason is that by using the makefile index.html is not regenerated in every run. Index.html therefore can't be the reference point for detecting changes .. it would trigger an endless loop of regeneration.

@arnehilmann
Copy link
Owner

I have the feeling that both our solutions are wrong somehow:
When a source file gets changed during the rendering, then it would not trigger a re-rendering, because the reference point gets set only after the current rendering finished. :o/
The reference point must be set at the beginning of a new rendering phase, at the beginning of the for-loop...
but for now, using a tmp-file should be enough, I think...
btw: there is a merge conflict in the loop script?!

@arnehilmann
Copy link
Owner

So what do I have to do to get the example deck rendered with the markdeck theme?
I can see the markdeck.scss file only, not the compiled css file.
And how are the fonts handled here?

@jceb
Copy link
Contributor Author

jceb commented Oct 3, 2019

I have the feeling that both our solutions are wrong somehow:
When a source file gets changed during the rendering, then it would not trigger a re-rendering, because the reference point gets set only after the current rendering finished. :o/
The reference point must be set at the beginning of a new rendering phase, at the beginning of the for-loop...

You're right, the temp file would need to be created before the build run to catch these changes. I'll update the loop accordingly.

btw: there is a merge conflict in the loop script?!

Ooops?!

@jceb
Copy link
Contributor Author

jceb commented Oct 3, 2019

So what do I have to do to get the example deck rendered with the markdeck theme?
I can see the markdeck.scss file only, not the compiled css file.
And how are the fonts handled here?

The way themes work is described here: https://github.com/hakimel/reveal.js#theming
So you create the file assets/css/theme/source/mytheme.scss. Additional files like images and fonts, I suggest to put in the assets/css/theme folder. During the build the whole assets folder gets copied to deck/assets/. Furthermore, assets/css/theme/source/mytheme.scss gets copied to deck/assets/3rdparty/reveal.js/css/theme/source/mytheme.scss and is compiled to deck/assets/3rdparty/reveal.js/css/theme/mytheme.css (notice that the theme is stored one folder up in the hierarchy).
The theme author just has to make sure that the path to additional resources is deck/assets/css/....

The advantage is that the whole revealjs theme mechanism is used which provides an easy way for updates. The only thing I don't like is that the path to the resources doesn't exist during development in assets/css/theme/source/*.scss.

What do you think about it?

@arnehilmann
Copy link
Owner

The way themes work is described here: https://github.com/hakimel/reveal.js#theming
So you create the file assets/css/theme/source/mytheme.scss. Additional files like images and fonts, I suggest to put in the assets/css/theme folder. During the build the whole assets folder gets copied to deck/assets/. Furthermore, assets/css/theme/source/mytheme.scss gets copied to deck/assets/3rdparty/reveal.js/css/theme/source/mytheme.scss and is compiled to deck/assets/3rdparty/reveal.js/css/theme/mytheme.css (notice that the theme is stored one folder up in the hierarchy).
The theme author just has to make sure that the path to additional resources is deck/assets/css/....

The advantage is that the whole revealjs theme mechanism is used which provides an easy way for updates. The only thing I don't like is that the path to the resources doesn't exist during development in assets/css/theme/source/*.scss.

What do you think about it?

I am on vacation right now (school holidays in Berlin/Brandenburg :o) and afk quite often.
But nevertheless some thoughts, because theming will be important IMHO:
a) using the reveals-theming mechanisms is okay as long as it is adaptable to other frameworks, too (here: impressjs has no theming support out-of-the-box, so that should be possible somehow)
b) the handling of referenced resources seems "suboptimal": putting all e.g. fonts in assets/css/theme mingles together all the resources from all themes; subfolder for each theme would be better, I think.
c) I would like a theming solution where it is possible to have a theme in both a subfolder or as a single archive file (markdeck copies a bunch of files from a to b, so that could be implemented there somewhere)
d) theming for markdeck slides should support markdeck-specifics, too: include-*-body, config values

Any thoughts/ideas/suggestions?

@arnehilmann
Copy link
Owner

last comment for today (I am afraid):
when using mktemp or the like, I recommend removing it in an EXIT trap, otherwise these files could pile up when the process gets interrupted in between (and wait-for-changes is a likely candidate for receiving a Ctrl-C)

@jceb
Copy link
Contributor Author

jceb commented Oct 6, 2019

a) using the reveals-theming mechanisms is okay as long as it is adaptable to other frameworks, too (here: impressjs has no theming support out-of-the-box, so that should be possible somehow)

Since impressjs has no theming support built-in, I see it as a challenge to build such support + make it compatible with revealjs. For impressjs the previously existing mechanism of customizing slides.scss still exists. My current attempt only focuses on revealjs. We could later on adapt it to other presentation frameworks.

b) the handling of referenced resources seems "suboptimal": putting all e.g. fonts in assets/css/theme mingles together all the resources from all themes; subfolder for each theme would be better, I think.

Totally agreed.

c) I would like a theming solution where it is possible to have a theme in both a subfolder or as a single archive file (markdeck copies a bunch of files from a to b, so that could be implemented there somewhere)

That would be optimal. However, I see challenges in making this compatible with standard revealjs themes. Since revealjs stores css files in css/theme/ and not in a subfolder, the best option I see is to store the theme as follows:

css/theme/<mytheme>.css
[css/theme/<mytheme>.scss] <- we could store the scss file also in this location, instead of the slighly annoying `../theme/source/` folder and copy it to the `../source/' folder for compilation.
css/theme/<mytheme>/images/*.jpg
css/theme/<mytheme>/fonts/*.woff
css/theme/<mytheme>/whatever/*

d) theming for markdeck slides should support markdeck-specifics, too: include-*-body, config values

That would actually be super. I guess we can source these files from css/theme/<mytheme>/include-*-body or css/theme/<mytheme>/include/*-body and .../defaults.yaml.

I'd say we first write the documentation for themes and then do the implementation. I'll open a new branch for it.

when using mktemp or the like, I recommend removing it in an EXIT trap, otherwise these files could pile up when the process gets interrupted in between (and wait-for-changes is a likely candidate for receiving a Ctrl-C)

👍

@jceb jceb mentioned this pull request Oct 7, 2019
@arnehilmann
Copy link
Owner

As mentioned in #38, v0.5 comes with themes support, but a bit different than implemented here.

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.

2 participants