Opened 4 years ago

Closed 4 years ago

Last modified 4 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: danielquinn 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 gabrielhurley)

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 4 years ago.
index.txt-16031.diff (527 bytes) - added by danielquinn 4 years ago.
Two inputs, one td, no csrf
index.txt-1603.1.diff (661 bytes) - added by danielquinn 4 years ago.
Added {% csrf_token %}
index.txt-1603.1.2.diff (739 bytes) - added by danielquinn 4 years ago.
Fixed the patch so that it can be applied from the root.
index.txt-1603.2.diff (1.4 KB) - added by danielquinn 4 years ago.
Includes fix for example.txt as well.
custom_comment_form-16031-2.diff (1.9 KB) - added by teraom 4 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 4 years ago by gabrielhurley

  • Description modified (diff)
  • Easy pickings set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 4 years ago by teraom

comment:2 Changed 4 years ago by teraom

  • Has patch set
  • Owner changed from nobody to teraom
  • Status changed from new to assigned

comment:3 Changed 4 years ago by julien

  • 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 4 years ago by danielquinn

Two inputs, one td, no csrf

comment:4 Changed 4 years ago by danielquinn

  • 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 4 years ago by danielquinn

  • Cc danielquinn added

Changed 4 years ago by danielquinn

Added {% csrf_token %}

comment:6 Changed 4 years ago by danielquinn

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 4 years ago by aaugustin

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 4 years ago by danielquinn

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

comment:8 Changed 4 years ago by julien

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 4 years ago by danielquinn

Includes fix for example.txt as well.

comment:9 Changed 4 years ago by danielquinn

  • Patch needs improvement unset

comment:10 Changed 4 years ago by julien

  • 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 4 years ago by teraom

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

comment:11 Changed 4 years ago by teraom

  • Patch needs improvement unset

comment:12 Changed 4 years ago by julien

  • Triage Stage changed from Accepted to Ready for checkin

That's great, thanks.

comment:13 Changed 4 years ago by jezdez

  • Resolution set to fixed
  • Status changed from assigned to closed

In [16412]:

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

comment:14 Changed 4 years ago by jezdez

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