honzakral/django-threadedcomments

A little fault in README and the python doc string: some unnecessary and extra `</li>` may occur.

njuaplusplus opened this issue · 1 comments

Here are the codes in your instrcution:

{% for comment in comment_list|fill_tree|annotate_tree %}
    {% ifchanged comment.parent_id %}{% else %}</li>{% endifchanged %}
    {% if not comment.open and not comment.close %}</li>{% endif %}
    {% if comment.open %}<ul>{% endif %}

    <li id="c{{ comment.id }}">
        ...
    {% for close in comment.close %}</li></ul>{% endfor %}
{% endfor %}

The 2nd line may have some faults.
If the comment.parent_id remains None, the ifchanged condition is False,
and it will output an </li>.
But if the comment is the root comment, that is, the comment is not a reply to an existing comment,
its parent_id is None.
Hence, there may be unnecessary </li> between the root comments.
As the browser is smart enough, it will removed the unmatched </li>,
so we may not be aware of this kind of bug.

However, if we use <div><div> instead of <ul><li>,
then the unnecessary </div> will mismatch other <div>.

So, I think the solution is to double check the comment.parent_id:

{% ifchanged comment.parent_id %}
{% else %}
    {% if comment.parent_id %}
    </li>
    {% endif %}
{% endifchanged %}

It is a bit like the issue #53
However it's not fixed completely by issue #54

Maybe we call the Case 3:

Case 3

Follow these steps:

  1. Post a comment.
  2. Post the 2nd comment. NOT a reply to the first comment.
  3. Post the 3rd comment. NOT a reply to the first comment.

Here is the tree:

  • Comment 1
  • Comment 2
  • Comment 3

Here is the HTML output (cleaned up):

<ul>
    <li>
        Comment 1
    </li>
</ul>
<ul>
    <li>
        Comment 2
    </li>
</ul>
</li><!-- This one -->
<ul>
    <li>
        Comment 3
    </li>
</ul>

Because when processing the comment 3 in the forloop,
its parent_id is same as comment 2 (None),
and it will emit a </li>