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

Use Jekyll/Liquid include for images/figures instead of Markdown #161

Open
rgaiacs opened this issue Aug 1, 2017 · 14 comments
Open

Use Jekyll/Liquid include for images/figures instead of Markdown #161

rgaiacs opened this issue Aug 1, 2017 · 14 comments
Assignees
Labels
type:discussion Discussion or feedback about the lesson type:enhancement Propose enhancement to the lesson

Comments

@rgaiacs
Copy link
Contributor

rgaiacs commented Aug 1, 2017

At the moment we are using "standard" Markdown syntax to include images/figures. Markdown syntax lacks some features such as caption. I want to propose we start using Jekyll/Liquid include for the next release.

Current behaviour

$ cat image.md 
![foo](foo.png 'foo text description')
$ kramdown image.md 
<p><img src="foo.png" alt="foo" title="foo text description" /></p>

Propose behaviour

$ cat image.md 
{% include figure.html filename="foo.png" caption="Caption describing foo" alternative_text="foo text description" %}
$ kramdown image.md 
<figure>
  <img
  src="foo.png"
  alt="foo text description">	
  <figcaption>Caption describing foo</figcaption>
</figure>

Required parameters

  • filename
  • alternative_text

Optional parameters

  • caption
  • license, preferable CC-BY
  • source_url, for any content that we didn't authored

Backward compatibility

This will not break backward compatibility. Any lesson that didn't migrate to use include will not see any difference.

@rgaiacs rgaiacs self-assigned this Aug 1, 2017
@gvwilson
Copy link
Contributor

gvwilson commented Aug 1, 2017

Can Kramdown parse these includes to give us a list of figures? If not, how will we regenerate _includes/all_figures.html ?

@rgaiacs
Copy link
Contributor Author

rgaiacs commented Aug 1, 2017

Can Kramdown parse these includes to give us a list of figures?

Jekyll uses Liquid for all the tags. We could change https://github.com/swcarpentry/lesson-example/blob/gh-pages/bin/markdown_ast.rb to do the tag processing replacement, see
https://github.com/Shopify/liquid/wiki/Liquid-for-Programmers, before kramdown do their job.

@brucellino
Copy link

brucellino commented Aug 1, 2017

how will we regenerate _includes/all_figures.html ?

I usually deal with this in a liquid loop.
Keep a yml file in _data (e.g. _data/figures.yml ) and then loop over them in the html :

{% for figure in site.data.figures %}
<img src="{{ figure.url }}" alt={{ figure.alt }}">
{% endfor %}

Forgive me if I'm misunderstanding something, but isn't this what you're looking for ?

@rgaiacs
Copy link
Contributor Author

rgaiacs commented Aug 1, 2017

@brucellino we don't keep a list of figures on any file because if authors need to edit more than one place we have a point of failure on our workflow. For that reason, we need a script to collect all the figures "in the correct order".

@brucellino
Copy link

Ok (sorry for barging in on this - I thought I had a good suggestion !). So, if I understand it correctly, you just want to be able to have the added features by including an image template (figures.html), the issue is not separating content from form. In that case, (and just for argument's sake) why not just write the image bits in html ? I can see that you want to avoid asking people to write in a format they are not comfortable with.

@rgaiacs
Copy link
Contributor Author

rgaiacs commented Aug 1, 2017

why not just write the image bits in html ?

Because this is error prone in the long run. Is the same reason why programmers write functions instead of duplicate code, if they need to change something they only need to change once. And as we need to eat our own dog food, using the template is the correct way. To give you a better example, if today we use CSS framework X and our HTML need to be

<figure class="x">
  <img
  src="foo.png"
  alt="foo text description">	
  <figcaption>Caption describing foo</figcaption>
</figure>

but tomorrow we use framework Y and our HTML need to be

<figure class="y">
  <img
  src="foo.png"
  alt="foo text description">	
  <figcaption>Caption describing foo</figcaption>
</figure>

we only need to make the change in one place, the template, instead of going in all the files looking for where we need to make the change.

@evanwill
Copy link
Contributor

evanwill commented Aug 2, 2017

Just to clarify @gvwilson, currently the _includes/all_figures.html is generated by the Makefile using the extract_figures.py. The advantage of that method is that it is grabbing all the figs in order and generating the all_figures. The disadvantage is that it is not part of the Jekyll build, so doesn't show up while developing with Jekyll.

We can reproduce the all_figures page with Liquid, including getting images in the correct order, by replacing _includes/all_figures.html with something like:

{% for item in site.episodes %}
{% assign lines = item.content | newline_to_br | split:'<br />' %}
{% for line in lines %}
{% if line contains '<img' %}
{{ line }}
{% endif %}
{% endfor %}
{% endfor %}

I like this sort of solution because it relies on one less python file, and is part of the Jekyll build process which makes development easier. It would work with both the current standard markdown images and with @rgaiacs figure include (assuming the img tag is on one line, rather than broken up like he displays it above), because it grabs the site.episodes after the .md files have been processed (i.e. they are converted to html and have their includes pulled in).

@rgaiacs
Copy link
Contributor Author

rgaiacs commented Aug 2, 2017

@evanwill Thanks for your feedback.

We can reproduce the all_figures page with Liquid, including getting images in the correct order, by replacing _includes/all_figures.html with something like: (...)

We can't use your code snippet because, as you mention on your comment, it will fail when we have

<img
src="foo.jpg"
>

which is a valid HTML code. This is the main reason that we are using the abstract syntax tree (AST) provided by kramdown, the same parser used by Jekyll.

Since most of the comments to my proposal so far diverted to the "List of Figures", I created another proposal, see #162, to discuss it.

@gcapes
Copy link
Contributor

gcapes commented Aug 2, 2017

I think this needs a cost-benefit analysis.

Markdown is easy to write. It's what we currently use and it works fine.

While image captions might be useful in some places, I don't feel that learning the new syntax and changing all the code is worth while. Some images will probably get missed or not work properly and someone will have to fix it.

@evanwill
Copy link
Contributor

evanwill commented Aug 2, 2017

@rgaiacs actually my Liquid snippet above will catch your figure include, even with the extra white space, since Liquid is looking at the page.content object in Jekyll, which standardizes the HTML since everything goes through the rendering engine. If you write a _include/figure.html like you suggest above, something like:

<figure>
  <img 
  src="{{site.url}}/fig/{{ include.file }}" 
  alt="{{ include.alt }}" >
  <figcaption>{{ include.caption }}</figcaption>
</figure>

And use it in a page like: {% include figure.html file="forking.svg" alt="cat pic" caption="My cat" %}

It will render without the extra white space, like:

<figure>
  <img src="http://localhost:4000/fig/forking.svg" alt="cat" />
  <figcaption>My cat</figcaption>
</figure>

Also, if the user writes some non-standard img tags into a markdown episode, it will still catch it, because kramdown normalizes the white space and tags.

My take is that a Jekyll project is a closed loop. You create the includes and set a few conventions, you don't need to parse for every possibility.

By adding a _include/figure.html you provide another option, but doesn't take away the current method of doing things. No one has to learn a new syntax unless they want to easily add a caption. The only objection was that the figure include doesn't work with the extract_figures.py to create all_figures.html. My Liquid proposal is simpler and actually more robust than the python script, since it is looking at the rendered html of the episodes.

@rgaiacs
Copy link
Contributor Author

rgaiacs commented Aug 3, 2017

@evanwill Thanks very much for all the clarification. 💖

@evanwill
Copy link
Contributor

evanwill commented Aug 3, 2017

ha ha, actually didn't mean to strongly argue for it, but I am working on some jekyll projects right now and have a sort of crush on Liquid.

In any case, I agree that having a _include/figure.html is helpful, and I see it on many other projects at the moment. Simplicity of writing and adapting the lesson content seems most important. Currently, adapting the styles and lesson-example repos seems overly complex. However, if we aren't making bigger changes to organizing the code and workflow, then introducing details like a fig include probably isn't necessary.

@fmichonneau
Copy link
Contributor

I'm -1 on this. Anything that deviates from Markdown syntax makes contributing more difficult, and reduces interoperability of the content.

@rgaiacs
Copy link
Contributor Author

rgaiacs commented Sep 9, 2017

After talk with other instructions we will merge this after a review of https://github.com/swcarpentry/styles/blob/gh-pages/bin/lesson_check.py and the documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:discussion Discussion or feedback about the lesson type:enhancement Propose enhancement to the lesson
Projects
None yet
Development

No branches or pull requests

6 participants