Posted by eliosh on September 8, 2008 at 10:47pm
| Project: | Feedback |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | active |
Issue Summary
It would be really nice to have a reponse for validation error.
If I enable captcha on form and give a wrong response, or even if i try to submit a empty message, feedback form remains in a "waiting" state, not giving any hint to user.
I try to write some code to enable this, but i think it'll be a big rework of JS code.
Thanks a lot for this wonderful module
Comments
#1
subscribe. thanks for the module!
#2
True. Should use drupal_to_json() as well as a dedicated menu item for AJAX form submissions, so we can intercept the page rendering and pass the validation error to our JS.
#3
subscribing
#4
I added an error function to the ajax call to display an error message if the POST fails.
#5
+++ feedback.css 31 Dec 2010 01:45:29 -0000@@ -68,6 +68,9 @@
#block-feedback-form #feedback-throbber {
+ min-height: 20px;
+}
+* html #block-feedback-form #feedback-throbber {
height: 20px;
}
How is this change related to this issue?
+++ feedback.css 31 Dec 2010 01:45:29 -0000
@@ -77,3 +80,6 @@
+#block-feedback-form #feedback-error-message {
+ color: #ff0000;
+}
+++ feedback.js 31 Dec 2010 01:45:29 -0000
@@ -55,25 +55,43 @@ Drupal.feedbackFormToggle = function ($b
+ error: function () {
+ Drupal.feedbackFormReset($form);
+ // Display error message.
+ $("#feedback-throbber").prepend('<div id="feedback-error-message">' + Drupal.t("An error occurred.") + '</div>');
It looks like this code only catches AJAX communication errors? Shouldn't it handle actual form validation errors?
I think we should have the server-side return actual Drupal error messages (i.e., from http://api.drupal.org/api/drupal/includes--bootstrap.inc/function/drupal... or theme_status_messages()) and display those.
In any case, i.e., regardless of whether we'll use theme_status_messages() or not, we should re-use the regular/normal markup and CSS classes for rendering error messages.
+++ feedback.js 31 Dec 2010 01:45:29 -0000@@ -55,25 +55,43 @@ Drupal.feedbackFormToggle = function ($b
+/**
+ * Reset the feedback form.
+ */
+Drupal.feedbackFormReset = function ($form) {
The name of this function confused me a bit, because "reset" normally refers to resetting form input fields to their initial state in JS. I understand it's hard to think of a better name, but perhaps simply appending "State" to the function name (feedbackFormResetState) might help. Of course, better ideas/suggestions are welcome :)
+++ feedback.js 31 Dec 2010 01:45:29 -0000@@ -55,25 +55,43 @@ Drupal.feedbackFormToggle = function ($b
+ $("#feedback-error-message").remove();
Only use double-quotes when technically required.
Powered by Dreditor.
#6
The CSS change is to make room for the error message.
The error message does display if the form does not validate (e.g. if the message was blank), although I'm not entirely clear on why that happens. I haven't figured out how to potentially get the real error messages back via AJAX, so this is kind of a makeshift approach.
Igree with using the same markup from theme_status_messages and the other tweaks.
#7
#8
#9
Let's focus on D7 first. The feedback form should use the AJAX framework.
#10
Full-blown AJAX magic with attached patch! :)
#11
I'd like to make progress to get to a D7 release hopefully soon. Thus, committed to HEAD.
Backport... unlikely. IMHO, waste of time. (AHAH in D6 is plain simply a nightmare)
#12
Automatically closed -- issue fixed for 2 weeks with no activity.
#13
closed #1139856: Feedback form does not handle error messages. and #1094082: Hangs when no text is sent! as duplicate.
Fixing this for D6 would be very nice.
#14
Unfortunately, there's no AJAX framework in D6, only a very poor #ahah construct requiring a shitload of custom code and manual testing to get it right. Due to that, a D6 version of this would look entirely different, and I never really intended to "backport" this code.
I'm trying to stay away from D6 as much as possible, so I'm not going to work on this myself. I can, however, understand people asking for it. Thus, leaving this issue open for some time to see whether anyone has the guts and required #ahah-fu to come up with a patch. I personally doubt it, but let's see. ;)
#15
Thanks for this handy module.
Is it possible to make the message field NOT "required" instead?