LAZR Javascript Library

Merge lp:~thumper/lazr-js/multi-line-editor-goodness into lp:lazr-js

  1. multi-line-editor-goodness
  2. Merge into toolchain
Proposed by Tim Penhey
Status: Merged
Merged at revision: 202
Proposed branch: lp:~thumper/lazr-js/multi-line-editor-goodness
Merge into: lp:lazr-js
Diff against target: 432 lines (+188/-52)
4 files modified
examples/inlineeditor/index.html (+29/-4)
src-js/lazrjs/inlineedit/assets/skins/sam/editor-skin.css (+13/-5)
src-js/lazrjs/inlineedit/editor.js (+44/-29)
src-js/lazrjs/inlineedit/tests/inline_edit.js (+102/-14)
To merge this branch: bzr merge lp:~thumper/lazr-js/multi-line-editor-goodness
Reviewer Review Type Date Requested Status
Deryck Hodge (community) code Approve
Review via email: mp+48118@code.launchpad.net

Commit message

Make the multiline editor more awesomer.

Description of the change

This branch attacks a number of really annoying bugs with the
multiline editor. Most of them CSS styling bugs.

  Bug 705548: Mousing over the edit button caused the rest of the page
    to be pushed down 1px.
  - this was solved by having the text-hover class have on less px bottom
    padding

  Bug 711054: extra trailing whitespace
  - solved by removing trailing whitespace in the value getter method

  Bug 711116: Cancel button bounces instead of eases on multiple enables
  - solved by removing the style left on the cancel button before showing it

  Bug 711124: Error font size is too small
  - removed the 0.5em on the error css class
  - I'm sure this was added as the initial implementation was to edit
    H1 tags, and the errors looked huge.

  Bug 711127: Multiline editor in error looks badly styled
  - Added a negative margin to the in-error css class
  - also tweaked the button label when in error to make it line up

  Bug 711128: Getting an error in a multiline editor disables resubmission
  - refactored the show and hide loading spinner in the editor.js file
  - added a hide loading spinner to the start of the show error method

Also, to aid testing, the editor example code was tweaked to make every
other call to the delayed save plugin fail and set an error.

To post a comment you must log in.
Revision history for this message
Deryck Hodge (deryck) wrote :

<deryck> thumper, so looks good to me. I would add a js test that the icons are re-enabled on error. And some text on the example page to explain that it will error every other attempt would be nice.
<deryck> thumper, r=me with those changes.
<thumper> deryck: ok

review: Approve (code)
lp:~thumper/lazr-js/multi-line-editor-goodness updated
207. By Tim Penhey

Add text to explain failures.

208. By Tim Penhey

Add some paragraph markers.

209. By Tim Penhey

Confirm that the editor's value has trailing whitespace stripped.

210. By Tim Penhey

Add a test to show that the submit and cancel buttons are shown when there is an error on save.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'examples/inlineeditor/index.html'
2--- examples/inlineeditor/index.html 2010-04-06 15:32:39 +0000
3+++ examples/inlineeditor/index.html 2011-02-07 01:22:12 +0000
4@@ -20,6 +20,11 @@
5 </script>
6
7 <style id="style-overrides" type="text/css">
8+
9+ body {
10+ padding: 10px;
11+ }
12+
13 #example {
14 margin-left: 20px;
15 padding: 50px 200px 50px 200px;
16@@ -30,12 +35,22 @@
17 font-size: 24pt;
18 color: brown;
19 }
20+
21+ #example .yui3-ieditor-errors {
22+ font-size: 0.5em;
23+ }
24 </style>
25 </head>
26 <body class="yui3-skin-sam">
27
28 <h1>Editing Text In-line</h1>
29
30+<h2>Saving behaviour</h2>
31+<p>In order to illustrate the error handling of the widgets, every other
32+attempt to save will cause an error to set on the widget. A second attempt to
33+save will succeed.
34+</p>
35+
36 <h2>Single-line</h2>
37 <div id="example">
38 <div id="editable_text">
39@@ -371,6 +386,8 @@
40 DelayedSavePlugin.superclass.constructor.apply(this, arguments);
41 };
42
43+ var failure_counter = 0;
44+
45 DelayedSavePlugin.NAME = 'delayedsave';
46 DelayedSavePlugin.NS = 'fx';
47
48@@ -416,11 +433,19 @@
49 // method.
50 Y.later(delay, this, function() {
51
52- Y.log("Running original 'save' method.", 'info');
53- this.original_save.apply(host, arguments);
54+ ++failure_counter;
55+ if (failure_counter % 2 == 0) {
56+ Y.log("Running original 'save' method.", 'info');
57+ this.original_save.apply(host, arguments);
58
59- // Make sure we clear the 'waiting' status.
60- host._uiClearWaiting();
61+ // Make sure we clear the 'waiting' status.
62+ host._uiClearWaiting();
63+ } else {
64+ Y.log("Failure...", 'info');
65+ host.showError("Some error occurred.");
66+ // Make sure we clear the 'waiting' status.
67+ host._uiClearWaiting();
68+ }
69
70 }, arguments);
71
72
73=== modified file 'src-js/lazrjs/inlineedit/assets/skins/sam/editor-skin.css'
74--- src-js/lazrjs/inlineedit/assets/skins/sam/editor-skin.css 2010-04-02 18:42:40 +0000
75+++ src-js/lazrjs/inlineedit/assets/skins/sam/editor-skin.css 2011-02-07 01:22:12 +0000
76@@ -3,7 +3,6 @@
77 .yui3-skin-sam .yui3-ieditor-errors {
78 padding: 0.2em 0 0.5em 0.5em;
79 font-family: sans-serif;
80- font-size: 0.5em;
81 color: red;
82 }
83
84@@ -11,6 +10,11 @@
85 background-color: #FFE4E4;
86 border: 4px solid red;
87 padding-right: 6px;
88+ margin: -5px -10px -4px -4px;
89+}
90+
91+.yui3-skin-sam .yui3-ieditor-in-error .bg-top-label {
92+ margin-top: 1px;
93 }
94
95 .yui3-skin-sam .yui3-ieditor-waiting .yui3-ieditor-btns button {
96@@ -42,7 +46,7 @@
97
98 .yui3-skin-sam .yui3-ieditor-multiline .yui3-ieditor-btns div {
99 /* Counteract the -4px indent of the editable text content body. */
100- margin-right: -4px;
101+ margin-right: -5px;
102 }
103
104 .yui3-skin-sam .yui3-ieditor-multiline .yui3-ieditor-btns .bg-top-label {
105@@ -123,17 +127,21 @@
106
107 .lazr-multiline-edit .yui3-editable_text-text {
108 border-top: 1px solid #d6d6d6;
109- padding:1px 10px 1px 20px;
110+ padding:5px 10px 3px 20px;
111 }
112
113 .lazr-multiline-edit .yui3-editable_text-text-hover {
114- padding:1px 9px 1px 19px;
115+ padding:5px 9px 2px 19px;
116 background-color: #fafafa !important;
117 border: 1px solid #d6d6d6;
118 }
119
120+.lazr-multiline-edit p {
121+ margin-bottom: 1.2em;
122+}
123+
124 .lazr-multiline-edit .yui3-ieditor-input {
125- padding:11px 3px 10px 18px;
126+ padding: 3px 9px 14px 17px;
127 border:0;
128 border: 1px solid #d6d6d6;
129 border-left: 2px groove #D6D6D6;
130
131=== modified file 'src-js/lazrjs/inlineedit/editor.js'
132--- src-js/lazrjs/inlineedit/editor.js 2011-01-14 15:50:06 +0000
133+++ src-js/lazrjs/inlineedit/editor.js 2011-02-07 01:22:12 +0000
134@@ -38,6 +38,8 @@
135
136 TOP_BUTTONS = 'top_buttons',
137 BOTTOM_BUTTONS = 'bottom_buttons',
138+ SUBMIT_BUTTON = 'submit_button',
139+ CANCEL_BUTTON = 'cancel_button',
140 BUTTONS = 'buttons',
141 B_TOP = 'top',
142 B_BOTTOM = 'bottom',
143@@ -498,7 +500,7 @@
144 // The multi-line editor has to dynamically resize,
145 // in case the widget is fluid.
146 var box_width = this.get(BOUNDING_BOX).get('offsetWidth');
147- var new_width = box_width - 25;
148+ var new_width = box_width - 29;
149 input.setStyle('width', new_width + 'px');
150 }
151
152@@ -553,6 +555,7 @@
153 * @param msg A string or HTMLElement to be displayed.
154 */
155 showError: function(msg) {
156+ this.hideLoadingSpinner();
157 this.get(ERROR_MSG).set('innerHTML', msg);
158 this.set(IN_ERROR, true);
159 this.get(INPUT_EL).focus();
160@@ -704,6 +707,8 @@
161 .addClass(C_CANCEL);
162 parent.appendChild(cancel);
163 parent.appendChild(ok);
164+ this.set(SUBMIT_BUTTON, ok);
165+ this.set(CANCEL_BUTTON, cancel);
166 },
167
168 /**
169@@ -801,6 +806,34 @@
170 msg.addClass(C_ERROR_HIDDEN);
171 },
172
173+ showLoadingSpinner: function(e) {
174+ // The multi-line editor submit icon should change to a spinner.
175+ if (this.get(MULTILINE)) {
176+ this.get(TOP_BUTTONS).one('.' + C_SUBMIT).setStyle(
177+ 'display', 'none');
178+ this.get(TOP_BUTTONS).one('.' + C_CANCEL).setStyle(
179+ 'display', 'none');
180+ var span = Y.Node.create('<span></span>');
181+ span.addClass(LOADING);
182+ e.target.get('parentNode').appendChild(span);
183+ }
184+ },
185+
186+ hideLoadingSpinner: function() {
187+ // Remove the spinner from the multi-line editor.
188+ if (this.get(MULTILINE)) {
189+ var spinner = this.get(TOP_BUTTONS).one('.' + LOADING);
190+ if (spinner) {
191+ var parent = spinner.get('parentNode');
192+ parent.removeChild(spinner);
193+ this.get(TOP_BUTTONS).one('.' + C_SUBMIT).setStyle(
194+ 'display', 'inline');
195+ this.get(TOP_BUTTONS).one('.' + C_CANCEL).setStyle(
196+ 'display', 'inline');
197+ }
198+ }
199+ },
200+
201 /**
202 * Bind the widget's DOM elements to their event handlers.
203 *
204@@ -812,16 +845,7 @@
205
206 this._bindButtons(C_SUBMIT, function(e) {
207 e.preventDefault();
208- // The multi-line editor submit icon should change to a spinner.
209- if (this.get(MULTILINE)) {
210- this.get(TOP_BUTTONS).one('.' + C_SUBMIT).setStyle(
211- 'display', 'none');
212- this.get(TOP_BUTTONS).one('.' + C_CANCEL).setStyle(
213- 'display', 'none');
214- var span = Y.Node.create('<span></span>');
215- span.addClass(LOADING);
216- e.target.get('parentNode').appendChild(span);
217- }
218+ this.showLoadingSpinner(e);
219 this.save();
220 });
221 this._bindButtons(C_CANCEL, function(e) {
222@@ -1075,7 +1099,9 @@
223 ptags.each(function(ptag) {
224 lines = lines.concat([ptag.get('text'), '\n\n']);
225 });
226- return lines.join("");
227+ var content = lines.join("");
228+ // Remove trailing whitespace.
229+ return content.replace(/\s+$/,'');
230 } else {
231 return this.get(TEXT).get('text');
232 }
233@@ -1150,10 +1176,9 @@
234 * @protected
235 */
236 _triggerEdit: function(e) {
237- //~ Y.log("Got trigger click", 'info');
238 e.preventDefault();
239 this.show_editor();
240- var cancel = this._editor_bb.one('.yui3-ieditor-cancel_button');
241+ var cancel = this._editor_bb.one('.' + C_CANCEL);
242 var anim = new Y.Anim({
243 node: cancel,
244 easing: Y.Easing.easeOut,
245@@ -1176,7 +1201,10 @@
246 * @method show_editor
247 */
248 show_editor: function() {
249- this.get(BOUNDING_BOX).addClass(C_EDIT_MODE);
250+ // Make sure that the cancel button starts back under the edit.
251+ var bounding_box = this.get(BOUNDING_BOX);
252+ bounding_box.one('.' + C_CANCEL).setStyle('left', 0);
253+ bounding_box.addClass(C_EDIT_MODE);
254 this.editor.set(VALUE, this.get(VALUE));
255 this.editor.syncUI();
256 this.editor.show();
257@@ -1398,7 +1426,6 @@
258 if (this.get(RENDERED)) {
259 var text = this.get(TEXT),
260 val = this.editor.get(VALUE);
261- //~ Y.log("Setting new text value '" + val + "' on " + text, 'info');
262 text.set('innerHTML', '');
263 if (this.editor.get(MULTILINE)) {
264 text.set('innerHTML', val);
265@@ -1436,16 +1463,7 @@
266 * @protected
267 */
268 _afterSave: function(e) {
269- // Remove the spinner from the multi-line editor.
270- if (this.editor.get(MULTILINE)) {
271- var spinner = this.editor.get(TOP_BUTTONS).one('.' + LOADING);
272- var parent = spinner.get('parentNode');
273- parent.removeChild(spinner);
274- this.editor.get(TOP_BUTTONS).one('.' + C_SUBMIT).setStyle(
275- 'display', 'inline');
276- this.editor.get(TOP_BUTTONS).one('.' + C_CANCEL).setStyle(
277- 'display', 'inline');
278- }
279+ this.editor.hideLoadingSpinner();
280 this.syncUI();
281 this.hide_editor();
282 this._uiAnimateSave();
283@@ -1461,7 +1479,6 @@
284 * @protected
285 */
286 _afterCancel: function(e) {
287- //~ Y.log("Got editor 'cancel' event", 'info');
288 this.hide_editor();
289 this._uiAnimateCancel();
290 },
291@@ -1496,8 +1513,6 @@
292
293 Y.EditableText = EditableText;
294
295-//~ Y.log("Module loaded", 'info', 'lazr.editor');
296-
297 }, "0.2", {"skinnable": true,
298 "requires": ["oop", "anim", "event", "node", "widget",
299 "lazr.anim", "lazr.base"]});
300
301=== modified file 'src-js/lazrjs/inlineedit/tests/inline_edit.js'
302--- src-js/lazrjs/inlineedit/tests/inline_edit.js 2011-01-14 17:13:05 +0000
303+++ src-js/lazrjs/inlineedit/tests/inline_edit.js 2011-02-07 01:22:12 +0000
304@@ -1,20 +1,20 @@
305 /* Copyright (c) 2009, Canonical Ltd. All rights reserved. */
306
307 YUI().use('lazr.editor', 'lazr.testing.runner', 'node',
308- 'event', 'event-simulate', 'console', function(Y) {
309-
310-var SAMPLE_HTML = " \
311- <h1>Single-line editing</h1> \
312- <div id='editable_single_text'> \
313- <span id='single_text' class='yui3-editable_text-text'>Some editable inline text.</span> \
314- <button id='single_edit' class='yui3-editable_text-trigger'>Edit this</button> \
315- </div> \
316- <hr /> \
317- <h1>Multi-line editing</h1> \
318- <div id='editable_multi_text'> \
319- <button id='multi_edit' class='yui3-editable_text-trigger'>Edit this</button> \
320- <span id='multi_text' class='yui3-editable_text-text'>Some editable multi-line text.</span> \
321- </div> \
322+ 'event', 'event-simulate', 'console', 'plugin', function(Y) {
323+var SAMPLE_HTML = " \
324+ <h1>Single-line editing</h1> \
325+ <div id='editable_single_text'> \
326+ <span id='single_text' class='yui3-editable_text-text'>Some editable inline text.</span> \
327+ <button id='single_edit' class='yui3-editable_text-trigger'>Edit this</button> \
328+ </div> \
329+ <hr /> \
330+ <h1>Multi-line editing</h1> \
331+ <div id='editable_multi_text'> \
332+ <button id='multi_edit' class='yui3-editable_text-trigger'>Edit this</button> \
333+ <span id='multi_text' class='yui3-editable_text-text'> \
334+ <p>Some editable multi-line text.</p></span> \
335+ </div> \
336 ";
337
338 var Assert = Y.Assert; // For easy access to isTrue(), etc.
339@@ -1156,6 +1156,94 @@
340 }
341 }));
342
343+
344+suite.add(new Y.Test.Case({
345+ name: "EditableText text value",
346+
347+ setUp: function() {
348+ setup_sample_html();
349+ this.multi = make_editable_text({
350+
351+ contentBox: '#editable_multi_text',
352+ multiline: true
353+ });
354+ this.multi.render();
355+ },
356+
357+ tearDown: function() {
358+ cleanup_widget(this.multi);
359+ },
360+
361+ test_text_value_no_trailing_newlines: function() {
362+ var text = this.multi.get('value');
363+ Assert.areEqual(
364+ "Some editable multi-line text.",
365+ text,
366+ "The editor kills trailing whitespace.");
367+ }
368+}));
369+
370+function FailedSavePlugin() {
371+ FailedSavePlugin.superclass.constructor.apply(this, arguments);
372+}
373+
374+FailedSavePlugin.NAME = 'failedsave';
375+FailedSavePlugin.NS = 'test';
376+
377+Y.extend(FailedSavePlugin, Y.Plugin.Base, {
378+ initializer: function(config) {
379+ this.doBefore("_saveData", this._altSave);
380+ },
381+
382+ _altSave: function() {
383+ var host = this.get('host');
384+ // Set the UI 'waiting' status.
385+ host._uiSetWaiting();
386+ host.showError("Some error occurred.");
387+ // Make sure we clear the 'waiting' status.
388+ host._uiClearWaiting();
389+ return new Y.Do.Halt();
390+ }
391+ });
392+
393+suite.add(new Y.Test.Case({
394+ name: "Edit buttons enabled on error",
395+
396+ setUp: function() {
397+ setup_sample_html();
398+ this.multi = make_editable_text({
399+
400+ contentBox: '#editable_multi_text',
401+ multiline: true
402+ });
403+ this.multi.render();
404+ this.multi.show_editor();
405+ },
406+
407+ tearDown: function() {
408+ cleanup_widget(this.multi);
409+ },
410+
411+ test_error_on_save_enabled_buttons: function() {
412+ var editor = this.multi.editor;
413+ editor.plug({fn:FailedSavePlugin});
414+ // Now saving should invoke an error.
415+ editor.save();
416+ Assert.isTrue(editor.get('in_error'), "Editor should be in error");
417+ // Both the submit and cancel buttons should be visible.
418+ Assert.areEqual(
419+ 'inline',
420+ editor.get('submit_button').getStyle('display'),
421+ "Submit should be set to display:inline");
422+ Assert.areEqual(
423+ 'inline',
424+ editor.get('cancel_button').getStyle('display'),
425+ "Cancel should be set to display:inline");
426+ }
427+}));
428+
429+
430+
431 Y.lazr.testing.Runner.add(suite);
432 Y.lazr.testing.Runner.run();
433

Subscribers

People subscribed via source and target branches