honzakral / django-threadedcomments

django-threadedcomments is a simple yet flexible threaded commenting system for Django.
BSD 3-Clause "New" or "Revised" License
622 stars 165 forks source link

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

Open njuaplusplus opened 8 years ago

njuaplusplus commented 8 years ago

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 %}
njuaplusplus commented 8 years ago

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:

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>