Issue 3589041: Add confirmation checkbox to Delete button (closes issue #252)
|
Created:
13 years, 4 months ago by techtonik Modified:
12 years, 6 months ago CC:
codereview-discuss_googlegroups.com Base URL:
http://rietveld.googlecode.com/svn/trunk/ Visibility:
Public. |
DescriptionBUG=http://code.google.com/p/rietveld/issues/detail?id=252
Patch Set 1 #Patch Set 2 : Updated for IE #Patch Set 3 : Alternative onsubmit works 100% #
Total comments: 3
MessagesTotal messages: 16
Doesn't this require changes to the view code that handles the POST request? On Sat, Dec 11, 2010 at 7:14 AM, <techtonik@gmail.com> wrote: > Reviewers: Andi Albrecht, > > > > Please review this at http://codereview.appspot.com/3589041/ > > Affected files: > M templates/edit.html > > > Index: templates/edit.html > =================================================================== > --- templates/edit.html (revision 623) > +++ templates/edit.html (working copy) > @@ -33,9 +33,11 @@ > {%ifequal issue.owner user%} > <h2>Delete This Issue</h2> > > -<form action="{%url codereview.views.delete issue.key.id%}" method="post"> > +<form action="{%url codereview.views.delete issue.key.id%}" > + method="post" name="delete"> > <input type="hidden" name="xsrf_token" value="{{xsrf_token}}"> > -<input type="submit" value="Delete Issue"> There is no undo! > +<input type="checkbox" name="confirm"> > +<input type="submit" value="Delete Issue" onclick="return > document.delete.confirm.checked"> There is no undo! > </form> > {%endifequal%} > > > > -- > You received this message because you are subscribed to the Google Groups > "codereview-discuss" group. > To post to this group, send email to codereview-discuss@googlegroups.com. > To unsubscribe from this group, send email to > codereview-discuss+unsubscribe@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/codereview-discuss?hl=en. > > -- --Guido van Rossum (python.org/~guido)
Sign in to reply to this message.
No. Delete button doesn't activate if onclick handler returns false. Copy/paste this form into http://www.tutorialspoint.com/cgi-bin/practice.cgi?file=javascript_1 and try yourself. <form method="post" name="delete" action=""> <input type="checkbox" name="confirm"> <input type="submit" value="Delete Issue" onclick="return document.delete.confirm.checked"> There is no undo! </form> On 2010/12/11 16:06:09, guido_python.org wrote: > Doesn't this require changes to the view code that handles the POST request? > > On Sat, Dec 11, 2010 at 7:14 AM, <mailto:techtonik@gmail.com> wrote: > > Reviewers: Andi Albrecht, > > > > > > > > Please review this at http://codereview.appspot.com/3589041/ > > > > Affected files: > > M templates/edit.html > > > > > > Index: templates/edit.html > > =================================================================== > > --- templates/edit.html (revision 623) > > +++ templates/edit.html (working copy) > > @@ -33,9 +33,11 @@ > > {%ifequal issue.owner user%} > > <h2>Delete This Issue</h2> > > > > -<form action="{%url codereview.views.delete issue.key.id%}" method="post"> > > +<form action="{%url codereview.views.delete issue.key.id%}" > > + method="post" name="delete"> > > <input type="hidden" name="xsrf_token" value="{{xsrf_token}}"> > > -<input type="submit" value="Delete Issue"> There is no undo! > > +<input type="checkbox" name="confirm"> > > +<input type="submit" value="Delete Issue" onclick="return > > document.delete.confirm.checked"> There is no undo! > > </form> > > {%endifequal%} > > > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "codereview-discuss" group. > > To post to this group, send email to mailto:codereview-discuss@googlegroups.com. > > To unsubscribe from this group, send email to > > mailto:codereview-discuss+unsubscribe@googlegroups.com. > > For more options, visit this group at > > http://groups.google.com/group/codereview-discuss?hl=en. > > > > > > > > -- > --Guido van Rossum (python.org/~guido)
Sign in to reply to this message.
Ok! On Sat, Dec 11, 2010 at 8:18 AM, <techtonik@gmail.com> wrote: > No. Delete button doesn't activate if onclick handler returns false. > > Copy/paste this form into > http://www.tutorialspoint.com/cgi-bin/practice.cgi?file=javascript_1 and > try yourself. > > <form method="post" name="delete" action=""> > <input type="checkbox" name="confirm"> > <input type="submit" value="Delete Issue" onclick="return > document.delete.confirm.checked"> There is no undo! > </form> > > > On 2010/12/11 16:06:09, guido_python.org wrote: >> >> Doesn't this require changes to the view code that handles the POST > > request? > >> On Sat, Dec 11, 2010 at 7:14 AM, <mailto:techtonik@gmail.com> wrote: >> > Reviewers: Andi Albrecht, >> > >> > >> > >> > Please review this at http://codereview.appspot.com/3589041/ >> > >> > Affected files: >> > M templates/edit.html >> > >> > >> > Index: templates/edit.html >> > =================================================================== >> > --- templates/edit.html (revision 623) >> > +++ templates/edit.html (working copy) >> > @@ -33,9 +33,11 @@ >> > {%ifequal issue.owner user%} >> > <h2>Delete This Issue</h2> >> > >> > -<form action="{%url codereview.views.delete issue.key.id%}" > > method="post"> >> >> > +<form action="{%url codereview.views.delete issue.key.id%}" >> > + method="post" name="delete"> >> > <input type="hidden" name="xsrf_token" value="{{xsrf_token}}"> >> > -<input type="submit" value="Delete Issue"> There is no undo! >> > +<input type="checkbox" name="confirm"> >> > +<input type="submit" value="Delete Issue" onclick="return >> > document.delete.confirm.checked"> There is no undo! >> > </form> >> > {%endifequal%} >> > >> > >> > >> > -- >> > You received this message because you are subscribed to the Google > > Groups >> >> > "codereview-discuss" group. >> > To post to this group, send email to > > mailto:codereview-discuss@googlegroups.com. >> >> > To unsubscribe from this group, send email to >> > mailto:codereview-discuss+unsubscribe@googlegroups.com. >> > For more options, visit this group at >> > http://groups.google.com/group/codereview-discuss?hl=en. >> > >> > > > > >> -- >> --Guido van Rossum (python.org/~guido) > > > > http://codereview.appspot.com/3589041/ > > -- > You received this message because you are subscribed to the Google Groups > "codereview-discuss" group. > To post to this group, send email to codereview-discuss@googlegroups.com. > To unsubscribe from this group, send email to > codereview-discuss+unsubscribe@googlegroups.com. > For more options, visit this group at > http://groups.google.com/group/codereview-discuss?hl=en. > > -- --Guido van Rossum (python.org/~guido)
Sign in to reply to this message.
Actually, the IE behaved not so good, so I've updated the patch. On 2010/12/11 16:37:15, guido_python.org wrote: > Ok! > > On Sat, Dec 11, 2010 at 8:18 AM, <mailto:techtonik@gmail.com> wrote: > > No. Delete button doesn't activate if onclick handler returns false. > > > > Copy/paste this form into > > http://www.tutorialspoint.com/cgi-bin/practice.cgi?file=javascript_1 and > > try yourself. > > > > <form method="post" name="delete" action=""> > > <input type="checkbox" name="confirm"> > > <input type="submit" value="Delete Issue" onclick="return > > document.delete.confirm.checked"> There is no undo! > > </form> > > > > > > On 2010/12/11 16:06:09, http://guido_python.org wrote: > >> > >> Doesn't this require changes to the view code that handles the POST > > > > request? > > > >> On Sat, Dec 11, 2010 at 7:14 AM, <mailto:techtonik@gmail.com> wrote: > >> > Reviewers: Andi Albrecht, > >> > > >> > > >> > > >> > Please review this at http://codereview.appspot.com/3589041/ > >> > > >> > Affected files: > >> > M templates/edit.html > >> > > >> > > >> > Index: templates/edit.html > >> > =================================================================== > >> > --- templates/edit.html (revision 623) > >> > +++ templates/edit.html (working copy) > >> > @@ -33,9 +33,11 @@ > >> > {%ifequal issue.owner user%} > >> > <h2>Delete This Issue</h2> > >> > > >> > -<form action="{%url codereview.views.delete issue.key.id%}" > > > > method="post"> > >> > >> > +<form action="{%url codereview.views.delete issue.key.id%}" > >> > + method="post" name="delete"> > >> > <input type="hidden" name="xsrf_token" value="{{xsrf_token}}"> > >> > -<input type="submit" value="Delete Issue"> There is no undo! > >> > +<input type="checkbox" name="confirm"> > >> > +<input type="submit" value="Delete Issue" onclick="return > >> > document.delete.confirm.checked"> There is no undo! > >> > </form> > >> > {%endifequal%} > >> > > >> > > >> > > >> > -- > >> > You received this message because you are subscribed to the Google > > > > Groups > >> > >> > "codereview-discuss" group. > >> > To post to this group, send email to > > > > mailto:codereview-discuss@googlegroups.com. > >> > >> > To unsubscribe from this group, send email to > >> > mailto:codereview-discuss+unsubscribe@googlegroups.com. > >> > For more options, visit this group at > >> > http://groups.google.com/group/codereview-discuss?hl=en. > >> > > >> > > > > > > > > >> -- > >> --Guido van Rossum (python.org/~guido) > > > > > > > > http://codereview.appspot.com/3589041/ > > > > -- > > You received this message because you are subscribed to the Google Groups > > "codereview-discuss" group. > > To post to this group, send email to mailto:codereview-discuss@googlegroups.com. > > To unsubscribe from this group, send email to > > mailto:codereview-discuss+unsubscribe@googlegroups.com. > > For more options, visit this group at > > http://groups.google.com/group/codereview-discuss?hl=en. > > > > > > > > -- > --Guido van Rossum (python.org/~guido)
Sign in to reply to this message.
Damn. I refuse to understand why this code works on http://www.tutorialspoint.com/cgi-bin/practice.cgi?file=javascript_1 but not it Rietveld? <html> <body> <form method="post" name="zed"> <input type="hidden" value="{{xsrf_token}}"> <input type="checkbox" id="confirm-delete2"> <input type="text" id="confirm-delete2"> <input type="submit" value="Delete Issue" onclick="return false;"> There is no undo! </form> </body> </html>
Sign in to reply to this message.
I think you need to add a onsubmit= on the form, not an onclick on the button. Although generally speaking, I'm not sure why your way doesn't work. That's a cool site BTW. You might be interested in jsfiddle.net which does things along the same lines, but I think is more powerful. On Sat, Dec 11, 2010 at 1:02 PM, <techtonik@gmail.com> wrote: > Damn. I refuse to understand why this code works on > http://www.tutorialspoint.com/cgi-bin/practice.cgi?file=javascript_1 > but not it Rietveld? > > > <html> > <body> > <form method="post" name="zed"> > <input type="hidden" value="{{xsrf_token}}"> > <input type="checkbox" id="confirm-delete2"> > <input type="text" id="confirm-delete2"> > <input type="submit" value="Delete Issue" onclick="return false;"> There > is no undo! > </form> > </body> > </html> > > > http://codereview.appspot.com/3589041/ > -- Ilia Mirkin imirkin@alum.mit.edu
Sign in to reply to this message.
I found the cause thanks to Chrome. It appears that our M_clickCommon handler (used only to hide help window) shadows code in onclick attribute for IE7. I'll commit onsubmit stuff if it looks ok for you (last patchset). BTW, thanks for jsfiddle.net - I've added it to bookmarks.
Sign in to reply to this message.
I think confirmation after you click delete is a bit more common than confirming before you click delete. Alternative change below. http://codereview.appspot.com/3589041/diff/10001/templates/edit.html File templates/edit.html (right): http://codereview.appspot.com/3589041/diff/10001/templates/edit.html#newcode36 templates/edit.html:36: <form action="{%url codereview.views.delete issue.key.id%}" method="post" I was thinking of something more like this. Note that the fixed width for the delete button is so that it doesn't change size when the user clicks so if they double click the button it won't hit the other button. <form action="{%url codereview.views.delete issue.key.id%}" method="post"> <input type="hidden" name="xsrf_token" value="{{xsrf_token}}"> <input id="pre-delete" type="button" value="Delete Issue" style="width:100" onclick="var x = document.getElementById('real-delete').style; x.display = x.display == '' ? 'none' : '' x = document.getElementById('pre-delete'); x.value = x.value == 'Cancel' ? 'Delete Issue' : 'Cancel';"> <input id="real-delete" type="submit" value="Confirm Delete - NO UNDO!" style="display:none"> </form>
Sign in to reply to this message.
http://jsfiddle.net/UAFp9/ Looks interesting, but to make it work on IE, we need to make our M_clickCommon handler to become active only when help window is visible. Otherwise M_clickCommon will be fired instead of your onclick event.
Sign in to reply to this message.
Email test, please ignore.
Sign in to reply to this message.
My IE is broken, so I won't be able to test any JS that is more complicated than my last patch set. So, any LGTM to proceed? Bruce, you will be able to submit your enhancement in a separate patch once we have the basic protection in place.
Sign in to reply to this message.
http://codereview.appspot.com/3589041/diff/10001/templates/edit.html File templates/edit.html (right): http://codereview.appspot.com/3589041/diff/10001/templates/edit.html#newcode39 templates/edit.html:39: <input type="checkbox" id="confirm-delete"> A plain checkbox before a button isn't really self-explaining IMO. Without knowledge of this review, I wouldn't know what to do with this checkbox and when it's not checked there's even no feedback when clicking the delete button. From a UI point of view I see no benefit in this change in the current form. What about adding at least some descriptive text like "Yes, I understand that there's NO UNDO." or something similar?
Sign in to reply to this message.
http://codereview.appspot.com/3589041/diff/10001/templates/edit.html File templates/edit.html (right): http://codereview.appspot.com/3589041/diff/10001/templates/edit.html#newcode39 templates/edit.html:39: <input type="checkbox" id="confirm-delete"> On 2011/07/07 19:27:43, Andi Albrecht wrote: > A plain checkbox before a button isn't really self-explaining IMO. Without > knowledge of this review, I wouldn't know what to do with this checkbox and when > it's not checked there's even no feedback when clicking the delete button. > > From a UI point of view I see no benefit in this change in the current form. > What about adding at least some descriptive text like "Yes, I understand that > there's NO UNDO." or something similar? I agree with Andi. My proposal was to instead add a button. When you click delete, a second button appears that says "Confirm Delete - NO UNDO!" and you have to press that to delete. (The delete button also changes to cancel so double click on delete does nothing.) See the code in my previous comment of 12/12/2010. Note that the Delete button is fixed width so it does not change size when the label changes to cancel. I can submit it as a separate patch if necessary or I'm happy for techtonik to submit.
Sign in to reply to this message.
This is a test, attempting to send email from guido@python.org. Expect it to fail.
Sign in to reply to this message.
On 2011/10/24 17:05:21, gvrpython wrote: > This is a test, attempting to send email from mailto:guido@python.org. Expect it to > fail. Apparently, it didn't.
Sign in to reply to this message.
|
