Merge lp:~salgado/launchpad/bug-415365 into lp:launchpad

[Guilherme Salgado]

[r=deryck,rharding][bug=415365] Use an overlay to display errors from inline multi-line text editors.

This uses an overlay to show errors on multi-line text editing (i.e. Y.EditableText) as the error is currently inserted in a node which is behind the text area so never seen by the user. I'm sure there's lots of room for improvement still, but this is certainly better than not showing the error to users at all, so I'd appreciate if that was taken into consideration.

[j.c.sackett (jcsackett)]

Including this screenshot https://launchpadlibrarian.net/94901859/overlay-error.png

[Richard Harding (rharding)]

You need to make sure to add lp.app.errors to the requirements of the module. This means that the tests html page will have to have it added as a dependency as well.

Then since the errors is going to require the overlay code, you'll need to add
lazr.formoverlay
lazr.overlay

to the test.html dependencies as well.

review: Needs Fixing (code*)

[Guilherme Salgado (salgado)]

Thanks for the quick feedback, Rick! I've done the changes you suggested and added a new test to show that display_error() is called on multi-line widgets.

[Richard Harding (rharding)]

Thanks, looks much better!

review: Approve (code*)

[Deryck Hodge (deryck)]

review: Approve

=== modified file 'lib/lp/app/javascript/inlineedit/editor.js'
--- lib/lp/app/javascript/inlineedit/editor.js	2011-12-21 07:54:40 +0000
+++ lib/lp/app/javascript/inlineedit/editor.js	2012-03-01 20:41:20 +0000
@@ -405,7 +405,11 @@
      */
     showError: function(msg) {
         this.hideLoadingSpinner();
-        this.get(ERROR_MSG).set('innerHTML', msg);
+        if (this.get(MULTILINE)) {
+            Y.lp.app.errors.display_error(this.get(BOUNDING_BOX), msg);
+        } else {
+            this.get(ERROR_MSG).set('innerHTML', msg);
+        }
         this.set(IN_ERROR, true);
         this.get(INPUT_EL).focus();
     },
@@ -1385,5 +1389,5 @@
 
 }, "0.2", {"skinnable": true,
            "requires": ["oop", "anim", "event", "node", "widget",
-                        "lp.anim", "lazr.base",
+                        "lp.anim", "lazr.base", "lp.app.errors",
                         "lp.app.formwidgets.resizing_textarea"]});
 
=== modified file 'lib/lp/app/javascript/inlineedit/tests/test_inline_edit.html'
--- lib/lp/app/javascript/inlineedit/tests/test_inline_edit.html	2012-02-16 20:35:40 +0000
+++ lib/lp/app/javascript/inlineedit/tests/test_inline_edit.html	2012-03-01 20:41:20 +0000
@@ -29,6 +29,12 @@
       <script type="text/javascript"
           src="../../../../../../build/js/lp/app/anim/anim.js"></script>
       <script type="text/javascript"
+          src="../../../../../../build/js/lp/app/errors.js"></script>
+      <script type="text/javascript"
+          src="../../../../../../build/js/lp/app/formoverlay/formoverlay.js"></script>
+      <script type="text/javascript"
+          src="../../../../../../build/js/lp/app/overlay/overlay.js"></script>
+      <script type="text/javascript"
           src="../../../../../../build/js/lp/app/lazr/lazr.js"></script>
       <script type="text/javascript"
           src="../../../../../../build/js/lp/app/extras/extras.js"></script>
 
=== modified file 'lib/lp/app/javascript/inlineedit/tests/test_inline_edit.js'
--- lib/lp/app/javascript/inlineedit/tests/test_inline_edit.js	2012-02-06 16:50:41 +0000
+++ lib/lp/app/javascript/inlineedit/tests/test_inline_edit.js	2012-03-01 20:41:20 +0000
@@ -243,6 +243,28 @@
                 "if there are no errors being displayed.");
         },
 
+        test_multiline_calls_display_error: function() {
+            this.editor.set('multiline', true);
+            this.editor.render();
+
+            // Ensure display_error is called.
+            var error_shown = false;
+            var old_error_method = Y.lp.app.errors.display_error;
+            Y.lp.app.errors.display_error = function(text) {
+                error_shown = true;
+            };
+
+            var msg = "An error has occured.";
+            this.editor.showError(msg);
+            Y.Assert.isTrue(error_shown);
+
+            // Restore original method.
+            Y.lp.app.errors.display_error = old_error_method;
+
+            // Restore to previous state.
+            this.editor.set('multiline', false);
+        },
+
         test_save_input_to_editor: function() {
             var expected_value = 'abc',
                 ed = this.editor;
@@ -1000,7 +1022,3 @@
     'lp.app.formwidgets.resizing_textarea', 'event',
     'event-simulate', 'plugin']
 });
-
-
-
-