org-static-blog icon indicating copy to clipboard operation
org-static-blog copied to clipboard

Fix org-static-blog-get-body logic

Open hellwolf opened this issue 2 years ago • 6 comments

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.

hellwolf avatar Oct 31 '22 22:10 hellwolf

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

bastibe avatar Nov 01 '22 10:11 bastibe

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

hellwolf avatar Nov 01 '22 11:11 hellwolf

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?

bastibe avatar Nov 01 '22 16:11 bastibe

Okay, I will have a look.

hellwolf avatar Nov 01 '22 18:11 hellwolf

~~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 avatar Nov 01 '22 18:11 hellwolf

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...

hellwolf avatar Nov 06 '22 15:11 hellwolf

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.

bastibe avatar Nov 07 '22 08:11 bastibe

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.

hellwolf avatar Nov 07 '22 08:11 hellwolf

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.

bastibe avatar Nov 07 '22 08:11 bastibe

Hi @bastibe,

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

hellwolf avatar Dec 11 '22 19:12 hellwolf

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!

hellwolf avatar Dec 11 '22 19:12 hellwolf

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.

bastibe avatar Dec 12 '22 06:12 bastibe

Yes, let's do it!

hellwolf avatar Dec 12 '22 08:12 hellwolf