launchpad-reviewers team mailing list archive

[Richard Harding <rick.harding@xxxxxxxxxxxxx>]

Richard Harding has proposed merging lp:~rharding/launchpad/resizing_textarea_919299 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #919299 in Launchpad itself: "resizing textarea in chrome in MP and blueprints inlineedit flip flops height"
  https://bugs.launchpad.net/launchpad/+bug/919299

For more details, see:
https://code.launchpad.net/~rharding/launchpad/resizing_textarea_919299/+merge/89515

= Summary =
There was an issue with the last bug fix that caused the 3rd change to a resizing textarea to resize to the min height for the widget. This then was reverted on the next resize event, and repeated over and over which has a lovely bouncing textarea effect.

== Proposed Fix ==
Correct the condition when we should make sure the textarea changes itself to the min height configured.

== Implementation Details ==
Basically the check should have been < and not !=

== Tests ==
Added a new test to verify the condition

google-chrome lib/lp/app/javascript/tests/test_beta_notification.html

== Demo and Q/A ==
Enter enough text into an inline edit widget that it hits the max size. Then make sure that it sticks there vs reverting between the max and min heights configured.

== Lint ==
Linting changed files:
  lib/lp/app/javascript/formwidgets/resizing_textarea.js
  lib/lp/app/javascript/formwidgets/tests/test_resizing_textarea.html
  lib/lp/app/javascript/formwidgets/tests/test_resizing_textarea.js
-- 
https://code.launchpad.net/~rharding/launchpad/resizing_textarea_919299/+merge/89515
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rharding/launchpad/resizing_textarea_919299 into lp:launchpad.

=== modified file 'lib/lp/app/javascript/formwidgets/resizing_textarea.js'
--- lib/lp/app/javascript/formwidgets/resizing_textarea.js	2012-01-18 13:54:33 +0000
+++ lib/lp/app/javascript/formwidgets/resizing_textarea.js	2012-01-21 00:41:24 +0000
@@ -280,7 +280,7 @@
             this.set_overflow();
 
             this._prev_scroll_height = scroll_height;
-        } else if (this._prev_scroll_height !== this.get('min_height')) {
+        } else if (this._prev_scroll_height < this.get('min_height')) {
             // This can occur when we go from hidden to not hidden via a
             // parent display setting. The initial height needs to get hit
             // again manually on display.

=== modified file 'lib/lp/app/javascript/formwidgets/tests/test_resizing_textarea.html'
--- lib/lp/app/javascript/formwidgets/tests/test_resizing_textarea.html	2012-01-18 13:46:13 +0000
+++ lib/lp/app/javascript/formwidgets/tests/test_resizing_textarea.html	2012-01-21 00:41:24 +0000
@@ -32,6 +32,7 @@
             style="max-width: 60em; width: 90%; display: block;" cols="44">
         </textarea>
     </div>
+    <textarea id="no_change" style="width: auto;"></textarea>
     <textarea id="one_line_sample" style="height: 1em; overflow: hidden; resize: none; width: auto;"></textarea>
     <textarea id="one_line" style="height: 1em; width: auto;"></textarea>
 </body>

=== modified file 'lib/lp/app/javascript/formwidgets/tests/test_resizing_textarea.js'
--- lib/lp/app/javascript/formwidgets/tests/test_resizing_textarea.js	2012-01-18 13:54:33 +0000
+++ lib/lp/app/javascript/formwidgets/tests/test_resizing_textarea.js	2012-01-21 00:41:24 +0000
@@ -168,6 +168,32 @@
             "The height should start out at 300px per the min_height config");
     },
 
+    test_height_stays_consistant: function () {
+        // once we adjust the height, another keystroke shouldn't move the
+        // height on us again, see bug #919299
+        var target = Y.one('#no_change');
+        target.plug(Y.lp.app.formwidgets.ResizingTextarea, {
+            skip_animations: true,
+            max_height: 200,
+            min_height: 100
+        });
+
+        update_content(target, test_text);
+        var new_height = get_height(target);
+        Y.Assert.areSame(200, new_height,
+            "The height should hit max at 200px");
+
+        update_content(target, test_text + "3");
+        var adjusted_height = get_height(target);
+        Y.Assert.areSame(200, adjusted_height,
+            "The height should still be at 200px");
+
+        update_content(target, test_text + "34");
+        var adjusted_height2 = get_height(target);
+        Y.Assert.areSame(200, adjusted_height2,
+            "The height should still be at 200px");
+    },
+
     test_oneline_should_size_to_single_em: function() {
         // Passing a one line in the cfg should limit the height to 1em even
         // though a normal textarea would be two lines tall.