bastibe / org-static-blog

A static site generator using org-mode
BSD 3-Clause "New" or "Revised" License
341 stars 74 forks source link

Fix org-static-blog-get-body logic #113

Closed hellwolf closed 1 year ago

hellwolf commented 2 years ago

As far as I understood, the logic should find where the postamble div is, assuming it is immediately after content div, then skip the comments div if any.

The fix is to correct this logic, without it when comments are enabled, it will make the index page look wrong when org-static-blog-use-preview nil.

bastibe commented 2 years ago

To be honest, I don't understand the issue. What exactly is rendered incorrectly on the index page?

hellwolf commented 2 years ago

In my case, it happens when (setq org-static-blog-post-comments "<div id=\"disqus_thread\"></div>") and org-static-blog-use-preview nil.

The post output would look like roughly this:

<div id="contents">
  <div class="taglist">...</div>
  <div id="comments"><div id="disqus_thread"></div></div>
</div>
<div id="postamble" class="status">

Without the fix, in the index page due to the org-static-blog-get-body logic, from the second post it would look like a unclosed <div> hence breaking the layout:

  <div class="taglist"> <--- not closed
bastibe commented 2 years ago

Thank you for the clarification. It seems that the post-extraction logic was not updated when we introduced the contents div.

Actually, we should probably move the comments-div out of contents, but that would potentially breaks existing CSS rules. I'd rather not do that without a good reason.

Alternatively, we could extract the entire contents, and scrape out the comments div afterwards manually. That's a bit more effort, but probably the better solution. This could be done as an additional step at the end of org-static-blog-get-body.

Would you like to take a stab at that?

hellwolf commented 2 years ago

Okay, I will have a look.

hellwolf commented 2 years ago

I do notice that the content divs themselves are not included in the index page. In the change you are proposing, would you want these content divs also be part of the page or?

Scratch that, let me dig a bit more.

hellwolf commented 2 years ago

Okay, I mean I refactored "org-static-blog-get-body" a bit with a two steps process. I am not sure if this is the best way approaching this. But I don't know idomatic way of processing it in emacs neither...

bastibe commented 2 years ago

I am confused. I think my previous comment was mistaken.

I had somehow assumed that the extracted content was supposed to include the <div id="content">, but looking at it now that doesn't seem to be the case. If that's true, your original commit was actually completely correct, I think. Right?

Although to avoid further confusion, we should probably change the function name org-static-blog-get-body to org-static-blog-get-content, and add a comment explaining that (search-backward "<div id=\"comments\">" nil t) is optional because comments are optional.

I am terribly sorry for the confusion I caused.

hellwolf commented 2 years ago

Okay. Don't worry, I am the beneficiary of your work rather, nothing to complain here from me. And it was fun to practice some elisp.

Change org-static-blog-get-body could indeed resolve some confusion, unless backward compatibility is an issue (affecting people who redefined the function name).

Let me know, I could restore the original code and add more comments, and perhaps change function name too.

bastibe commented 2 years ago

If you'd like to do that, I'd be grateful!

And go ahead with the function name change. If it's not part of the customize interface, I consider it internal.

hellwolf commented 1 year ago

Hi @bastibe,

I am finally back to this. Please have a look now!

hellwolf commented 1 year ago

Note that org-static-blog-get-post-content could be a breaking change to people actually customize the function, but maybe flagging it in the commit message is sufficient for people to find out!

bastibe commented 1 year ago

Thank you for getting back to this!

The pull request looks good to me. I'll happily merge it when you think it's ready.

hellwolf commented 1 year ago

Yes, let's do it!