|
|
|
Created:
15 years, 1 month ago by techtonik Modified:
14 years, 2 months ago CC:
codereview-discuss_googlegroups.com Base URL:
http://rietveld.googlecode.com/svn/trunk/ Visibility:
Public. |
BUG=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
Total messages: 16
|
techtonik
|
15 years, 1 month ago (2010年12月11日 15:14:13 UTC) #1 |
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)
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)
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)
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)
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>
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
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.
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>
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.
Email test, please ignore.
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.
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?
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.
This is a test, attempting to send email from guido@python.org. Expect it to fail.
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.