Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#16031 closed Cleanup/optimization (fixed)

Custom comment form is incomplete and wrong.

Reported by: ddshore@… Owned by: teraom
Component: Documentation Version: 1.3
Severity: Normal Keywords:
Cc: Daniel Quinn Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description (last modified by Gabriel Hurley)

When viewing the documentation for the django comment framework (http://docs.djangoproject.com/en/dev/ref/contrib/comments/) this is the example form that is shown:

{% get_comment_form for event as form %}
<form action="{% comment_form_target %}" method="post">
  {{ form }}
  <tr>
    <td></td>
    <td><input type="submit" name="preview" class="submit-post" value="Preview"></td>
  </tr>
</form>

However, the post button is missing, only the preview button appears, and it's class is shown as a post button. Not sure what the original intention was, but it would probably be better to have both buttons in the documentation:

	<input type = "submit" name = "post" class = "submit-post" value = "post">
	<input type = "submit" name = "preview" class = "submit-preview" value = "preview">

or at least correct the class for the preview button to submit-preview instead of submit-post.

Attachments (6)

custom_comment_form-16031.diff (590 bytes) - added by teraom 6 years ago.
index.txt-16031.diff (527 bytes) - added by Daniel Quinn 5 years ago.
Two inputs, one td, no csrf
index.txt-1603.1.diff (661 bytes) - added by Daniel Quinn 5 years ago.
Added {% csrf_token %}
index.txt-1603.1.2.diff (739 bytes) - added by Daniel Quinn 5 years ago.
Fixed the patch so that it can be applied from the root.
index.txt-1603.2.diff (1.4 KB) - added by Daniel Quinn 5 years ago.
Includes fix for example.txt as well.
custom_comment_form-16031-2.diff (1.9 KB) - added by teraom 5 years ago.
Changed to one tr tag with colspan 2. Removed all class attributes. Wrapped form in table tag

Download all attachments as: .zip

Change History (20)

comment:1 Changed 6 years ago by Gabriel Hurley

Description: modified (diff)
Easy pickings: set
Triage Stage: UnreviewedAccepted

Agreed. There are definitely some mixed signals in that example. Might as well include both buttons for completeness.

Speaking of "preview" buttons, you might want to use Trac's preview to check your code formatting before submitting your ticket next time. ;-)

Changed 6 years ago by teraom

comment:2 Changed 6 years ago by teraom

Has patch: set
Owner: changed from nobody to teraom
Status: newassigned

comment:3 Changed 6 years ago by Julien Phalip

Patch needs improvement: set

The patch introduces a third column, which is not correct since the form only has two columns. It would be more correct to have a single <td colspan="2">.

For completeness it'd also be good to wrap the form with <table></table> tags, and also to clean up a similar example in https://docs.djangoproject.com/en/dev/ref/contrib/comments/example/

Changed 5 years ago by Daniel Quinn

Attachment: index.txt-16031.diff added

Two inputs, one td, no csrf

comment:4 Changed 5 years ago by Daniel Quinn

UI/UX: unset

I've attached a patch that should solve the problem, however it doesn't include a {% csrf_token %} even though it needs to be there if the form is to work at all. Should I include that as well, or leave it as is?

comment:5 Changed 5 years ago by Daniel Quinn

Cc: Daniel Quinn added

Changed 5 years ago by Daniel Quinn

Attachment: index.txt-1603.1.diff added

Added {% csrf_token %}

comment:6 Changed 5 years ago by Daniel Quinn

After talking to one of the Elder Nerds here at DjangoCon, we agreed that the {% csrf_token %} should be in there, so I've added a new patch.

comment:7 Changed 5 years ago by Aymeric Augustin

Could you make your patch relative to the trunk directory of the svn repo? Like this: cd trunk; svn di > your_patch.diff.

We expect to be able to apply it with cd trunk; patch -p0 < your_patch.diff (or -p1 if you're using git).

Also, when you upload a new patch that take into account the reviewers' remarks, you can uncheck "patch needs improvement".

Thanks!

Changed 5 years ago by Daniel Quinn

Attachment: index.txt-1603.1.2.diff added

Fixed the patch so that it can be applied from the root.

comment:8 Changed 5 years ago by Julien Phalip

The code snippet just above the "Flagging" section in https://docs.djangoproject.com/en/dev/ref/contrib/comments/example/ should also be fixed as part of this ticket.

Changed 5 years ago by Daniel Quinn

Attachment: index.txt-1603.2.diff added

Includes fix for example.txt as well.

comment:9 Changed 5 years ago by Daniel Quinn

Patch needs improvement: unset

comment:10 Changed 5 years ago by Julien Phalip

Patch needs improvement: set

This looks good. Just one more change to remove any confusion: let's completely remove all instances of class="submit-post"

Changed 5 years ago by teraom

Changed to one tr tag with colspan 2. Removed all class attributes. Wrapped form in table tag

comment:11 Changed 5 years ago by teraom

Patch needs improvement: unset

comment:12 Changed 5 years ago by Julien Phalip

Triage Stage: AcceptedReady for checkin

That's great, thanks.

comment:13 Changed 5 years ago by Jannis Leidel

Resolution: fixed
Status: assignedclosed

In [16412]:

Fixed #16031 -- Corrected comments template examples. Thanks, teraom.

comment:14 Changed 5 years ago by Jannis Leidel

In [16421]:

[1.3.X] Fixed #16031 -- Corrected comments template examples. Thanks, teraom.

Backport from trunk (r16412).

Note: See TracTickets for help on using tickets.
Back to Top