Merge lp:~jcsackett/launchpad/simplify-everything into lp:launchpad

Proposed by j.c.sackett
Status: Merged
Approved by: Curtis Hovey
Approved revision: no longer in the source branch.
Merged at revision: 15304
Proposed branch: lp:~jcsackett/launchpad/simplify-everything
Merge into: lp:launchpad
Diff against target: 353 lines (+96/-92)
7 files modified
lib/lp/app/javascript/banners/banner.js (+14/-22)
lib/lp/app/javascript/banners/beta-notification.js (+14/-14)
lib/lp/app/javascript/banners/privacy.js (+7/-4)
lib/lp/app/javascript/banners/tests/test_banner.js (+10/-19)
lib/lp/app/javascript/banners/tests/test_privacy.js (+14/-0)
lib/lp/app/templates/banner-macros.pt (+27/-23)
lib/lp/bugs/templates/bugtarget-macros-filebug.pt (+10/-10)
To merge this branch: bzr merge lp:~jcsackett/launchpad/simplify-everything
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Review via email: mp+107240@code.launchpad.net

Commit message

Simplifies the progressive enhancement and other uses of the global-notification banners.

Description of the change

Summary
=======
The previous work to make the banners work without js involved using an overly
complicated renderUI sequence that would replace existing HTML with the new
banner, but would have multiple banner objects. This branch massively
simplifies things by removing that bit of goofiness and using YUI's widget
srcNode attribute to safely reuse the html.

UPDATE: This also fixes a regression introduced by earlier banner work; after
the first banner.js branch landed +filebug was special cased in such a way that
it failed to work properly with private-by-default projects. Since this removes
the special casing, it has restored the functionality.

Preimp
======
Continuation of earlier work discussed with Curtis Hovey and Rick Harding.

Implementation
==============
The _createBanner and _updateBanner methods are discarded. _createBanner is
merged with renderUI; _updateBanner becomes updateText, since that's really
all it does.

The banner_id attribute and its uses are removed, most notably in the template
banner.

The beta banner is switched to using the same singleton banner as the privacy
banner, since it too exists as only one instance per page.

Both the beta banner and privacy banner are updated to get srcNode within
their wrapper functions. This can be null, b/c Y.Widget handles null srcNodes,
which just indicates that there's no html to update.

The privacy banner wrapper has been updated to use updateText to set new
banner texts instead of the config on the initializer. All calls to the
PrivacyBanner now use the wrapper.

test_only_one_banner is deleted, b/c it's not needed.

test_updateText is added.

test_privacy gets a test_only_one banner, which uses the wrapper function to
verify that only banner is created.

The html in banner-macros is updated to correspond exactly to the html
generated by the widgets, less the dynamically generated IDs from YUI.

Tests
=====
bin/test -vvc -t beta -t banner -t privacy -t type_choice --layer=YUI

QA
==
Confirm that no-js and js use of privacy and beta banners doesn't change.

Confirm that the banner shows up on +filebug for private-by-default projects.

LoC
===
This is part of disclosure work.

Lint
====

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/app/javascript/banners/beta-notification.js
  lib/lp/app/javascript/banners/tests/test_banner.js
  lib/lp/app/javascript/banners/banner.js
  lib/lp/bugs/templates/bugtarget-macros-filebug.pt
  lib/lp/app/templates/banner-macros.pt
  lib/lp/app/javascript/banners/privacy.js
  lib/lp/app/javascript/banners/tests/test_privacy.js

./lib/lp/app/templates/banner-macros.pt
      41: mismatched tag

I believe this lint error is wrong; line 41 is a closing div that looks
matched. The reviewr is invited to tell me I'm wrong if they can see the
issue, in which case I'll fix it. I just don't see it, and macro compilation
still works.

To post a comment you must log in.
Curtis Hovey (sinzui) wrote :

Lovely thank you.

review: Approve (code)
Curtis Hovey (sinzui) wrote :

Oh, I see the link error. the second to last <span> tag is OPEN. It should be CLOSED.

j.c.sackett (jcsackett) wrote :

Thanks, fixed that!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/lp/app/javascript/banners/banner.js'
2--- lib/lp/app/javascript/banners/banner.js 2012-05-18 20:10:14 +0000
3+++ lib/lp/app/javascript/banners/banner.js 2012-05-24 20:50:26 +0000
4@@ -93,30 +93,15 @@
5 login_space.run();
6 },
7
8- _createBanner: function () {
9+ renderUI: function () {
10 var banner_data = {
11 badge: this.get('banner_icon'),
12 text: this.get('notification_text'),
13- banner_id: this.get('banner_id')
14 };
15- return Y.lp.mustache.to_html(
16+ var banner_html = Y.lp.mustache.to_html(
17 this.get('banner_template'),
18 banner_data);
19- },
20-
21- _updateBanner: function (banner_node) {
22- text_node = banner_node.one('.banner-text');
23- text_node.set('innerText', this.get('notification_text'));
24- },
25-
26- renderUI: function() {
27- var existing_banner = Y.one('#' + this.get('banner_id'));
28- if (existing_banner) {
29- this._updateBanner(existing_banner);
30- } else {
31- var banner_html = this._createBanner();
32- this.get('contentBox').append(banner_html);
33- }
34+ this.get('contentBox').append(banner_html);
35 },
36
37 bindUI: function() {
38@@ -127,17 +112,24 @@
39 this._hideBanner();
40 }
41 });
42- }
43+ },
44+
45+ updateText: function (new_text) {
46+ if (!Y.Lang.isValue(new_text)) {
47+ new_text = this.get('notification_text');
48+ }
49+ text_node = this.get('contentBox').one('.banner-text');
50+ text_node.set('innerText', new_text);
51+ },
52+
53 }, {
54 ATTRS: {
55- banner_id: { value: "base-banner" },
56 notification_text: { value: "" },
57 banner_icon: { value: "<span></span>" },
58 banner_template: {
59 valueFn: function() {
60 return [
61- '<div id="{{ banner_id }}"',
62- 'class="global-notification transparent hidden">',
63+ '<div class="global-notification transparent hidden">',
64 '{{{ badge }}}',
65 '<span class="banner-text">{{ text }}</span>',
66 "</div>"].join('')
67
68=== modified file 'lib/lp/app/javascript/banners/beta-notification.js'
69--- lib/lp/app/javascript/banners/beta-notification.js 2012-05-18 22:26:44 +0000
70+++ lib/lp/app/javascript/banners/beta-notification.js 2012-05-24 20:50:26 +0000
71@@ -5,20 +5,16 @@
72
73 ns.BetaBanner = Y.Base.create('betaBanner', baseBanner, [], {
74
75- _createBanner: function () {
76+ renderUI: function () {
77 var banner_data = {
78 badge: this.get('banner_icon'),
79 text: this.get('notification_text'),
80 features: this.get('features'),
81- banner_id: this.get('banner_id')
82 };
83- return Y.lp.mustache.to_html(
84+ var banner_html = Y.lp.mustache.to_html(
85 this.get('banner_template'),
86 banner_data);
87- },
88-
89- renderUI: function () {
90- baseBanner.prototype.renderUI.apply(this, arguments);
91+ this.get('contentBox').append(banner_html);
92 var beta_node = Y.one('.global-notification');
93 var close_box = Y.Node.create(
94 '<a href="#" class="global-notification-close">Hide' +
95@@ -38,14 +34,12 @@
96
97 }, {
98 ATTRS: {
99- banner_id: { value: "beta-banner" },
100 notification_text: { value: "Some parts of this page are in beta: " },
101 banner_icon: { value: '<span class="beta-warning">BETA!</span>' },
102 banner_template: {
103 valueFn: function() {
104 return [
105- '<div id="{{ banner_id }}"',
106- 'class="global-notification transparent hidden">',
107+ '<div class="global-notification transparent hidden">',
108 '{{{ badge }}}',
109 '<span class="banner-text">',
110 '{{ text }}{{{ features }}}',
111@@ -72,11 +66,17 @@
112 }
113 });
114
115+// For the beta banner to work, it needs to have one instance, and one
116+// instance only.
117+_singleton_beta_banner = null;
118 ns.show_beta_if_needed = function () {
119- var banner = new ns.BetaBanner();
120- if (banner.get('features').length !== 0) {
121- banner.render();
122- banner.show();
123+ if (_singleton_beta_banner === null) {
124+ var src = Y.one('.yui3-betabanner')
125+ _singleton_beta_banner = new ns.BetaBanner({ srcNode: src });
126+ }
127+ if (_singleton_beta_banner.get('features').length !== 0) {
128+ _singleton_beta_banner.render();
129+ _singleton_beta_banner.show();
130 }
131 }
132
133
134=== modified file 'lib/lp/app/javascript/banners/privacy.js'
135--- lib/lp/app/javascript/banners/privacy.js 2012-05-17 21:04:18 +0000
136+++ lib/lp/app/javascript/banners/privacy.js 2012-05-24 20:50:26 +0000
137@@ -5,7 +5,6 @@
138
139 ns.PrivacyBanner = Y.Base.create('privacyBanner', baseBanner, [], {}, {
140 ATTRS: {
141- banner_id: { value: "privacy-banner" },
142 notification_text: {
143 value: "The information on this page is private."
144 },
145@@ -18,12 +17,16 @@
146 // For the privacy banner to work, it needs to have one instance, and one
147 // instance only.
148 var _singleton_privacy_banner = null;
149-
150-ns.getPrivacyBanner = function () {
151+ns.getPrivacyBanner = function (banner_text) {
152+
153 if (_singleton_privacy_banner === null) {
154- _singleton_privacy_banner = new ns.PrivacyBanner();
155+ var src = Y.one('.yui3-privacybanner')
156+ _singleton_privacy_banner = new ns.PrivacyBanner({ srcNode: src });
157 _singleton_privacy_banner.render();
158 }
159+ if (Y.Lang.isValue(banner_text)) {
160+ _singleton_privacy_banner.updateText(banner_text);
161+ }
162 return _singleton_privacy_banner;
163 }
164
165
166=== modified file 'lib/lp/app/javascript/banners/tests/test_banner.js'
167--- lib/lp/app/javascript/banners/tests/test_banner.js 2012-05-22 21:31:04 +0000
168+++ lib/lp/app/javascript/banners/tests/test_banner.js 2012-05-24 20:50:26 +0000
169@@ -62,7 +62,6 @@
170 banner.render();
171
172 var banner_node = Y.one(".global-notification");
173- console.log(banner_node);
174 var badge = banner_node.one('.sprite');
175 Y.Assert.isObject(banner_node);
176 Y.Assert.areEqual(cfg.notification_text, banner_node.get('text'));
177@@ -72,8 +71,7 @@
178 test_show: function() {
179 var banner = new Y.lp.app.banner.Banner({ skip_animation: true });
180 banner.render();
181- banner.show();
182-
183+ banner.show();
184 var banner_node = Y.one(".global-notification");
185 Y.Assert.isFalse(banner_node.hasClass('hidden'));
186 },
187@@ -94,25 +92,18 @@
188 this.wait(check, wait_for_anim);
189 },
190
191- test_only_one_banner: function () {
192- // There can only be one banner node for a given type of banner.
193+ test_updateText: function() {
194 var banner = new Y.lp.app.banner.Banner({ skip_animation: true });
195 banner.render();
196- banner.show();
197- Y.Assert.areEqual(1, Y.all('.global-notification').size());
198+ var new_text = 'some new text';
199+ banner.updateText(new_text);
200+ var banner_node = Y.one(".global-notification");
201+ Y.Assert.areEqual(new_text, banner_node.get('text'));
202
203- var new_text = 'This is new text';
204- var banner2 = new Y.lp.app.banner.Banner({
205- notification_text: new_text,
206- skip_animation: true
207- });
208- banner2.render();
209- banner2.show();
210- Y.Assert.areEqual(1, Y.all('.global-notification').size());
211- var banner_node = Y.one('.global-notification');
212- Y.Assert.areEqual(
213- new_text,
214- Y.one('.global-notification').get('text'));
215+ banner.updateText();
216+ var banner_node = Y.one(".global-notification");
217+ Y.Assert.areEqual("", banner_node.get('text'));
218 }
219 }));
220+
221 }, '0.1', {'requires': ['test', 'console', 'lp.app.banner']});
222
223=== modified file 'lib/lp/app/javascript/banners/tests/test_privacy.js'
224--- lib/lp/app/javascript/banners/tests/test_privacy.js 2012-05-22 21:31:04 +0000
225+++ lib/lp/app/javascript/banners/tests/test_privacy.js 2012-05-24 20:50:26 +0000
226@@ -36,6 +36,20 @@
227 '<span class="sprite notification-private"></span>',
228 banner.get('banner_icon'));
229 },
230+
231+ test_only_one_banner: function () {
232+ // getPrivacyBanner only returns one banner.
233+ var banner = Y.lp.app.banner.privacy.getPrivacyBanner();
234+ Y.Assert.areEqual(1, Y.all('.global-notification').size());
235+
236+ var new_text = 'This is new text';
237+ var banner = Y.lp.app.banner.privacy.getPrivacyBanner(new_text);
238+ Y.Assert.areEqual(1, Y.all('.global-notification').size());
239+ var banner_node = Y.one('.global-notification');
240+ Y.Assert.areEqual(
241+ new_text,
242+ Y.one('.global-notification').get('text'));
243+ }
244 }));
245
246 }, '0.1', {'requires': ['test', 'console', 'lp.app.banner.privacy']});
247
248=== modified file 'lib/lp/app/templates/banner-macros.pt'
249--- lib/lp/app/templates/banner-macros.pt 2012-05-21 20:12:08 +0000
250+++ lib/lp/app/templates/banner-macros.pt 2012-05-24 20:50:26 +0000
251@@ -9,35 +9,39 @@
252
253 <metal:privacy define-macro="privacy-banner">
254 <tal:show-banner condition="view/private">
255- <div class="yui3-privacybanner-content">
256- <div id="privacy-banner" class="global-notification">
257- <span class="sprite notification-private"></span>
258- <span class="banner-text">The information on this page is private.</span>
259+ <div class="yui3-widget yui3-banner yui3-privacybanner">
260+ <div class="yui3-privacybanner-content">
261+ <div class="global-notification">
262+ <span class="sprite notification-private"></span>
263+ <span class="banner-text">The information on this page is private.</span>
264+ </div>
265 </div>
266 </div>
267 </tal:show-banner>
268 </metal:privacy>
269
270 <metal:beta define-macro="beta-banner">
271-<tal:show-banner condition="view/beta_features">
272-<div class="yui3-betabanner-content">
273- <div id="beta-banner" class="global-notification">
274- <span class="beta-warning">BETA!</span>
275- <span class="banner-text">
276- Some parts of this page are in beta:&nbsp;
277- <span class="beta-feature">
278- <tal:features
279- repeat="feature view/beta_features">
280- <tal:feature replace="feature/title" />
281- <tal:link condition="feature/url">
282- (<a tal:attributes="href feature/url" class="info-link">read more</a>)
283- </tal:link>
284- </tal:features>
285- <span>
286- </span>
287- </div>
288-</div>
289-</tal:show-banner>
290+ <tal:show-banner condition="view/beta_features">
291+ <div class="yui3-widget yui3-banner yui3-betabanner">
292+ <div class="yui3-betabanner-content">
293+ <div class="global-notification">
294+ <span class="beta-warning">BETA!</span>
295+ <span class="banner-text">
296+ Some parts of this page are in beta:&nbsp;
297+ <span class="beta-feature">
298+ <tal:features
299+ repeat="feature view/beta_features">
300+ <tal:feature replace="feature/title" />
301+ <tal:link condition="feature/url">
302+ (<a tal:attributes="href feature/url" class="info-link">read more</a>)
303+ </tal:link>
304+ </tal:features>
305+ </span>
306+ </span>
307+ </div>
308+ </div>
309+ </div>
310+ </tal:show-banner>
311 </metal:beta>
312
313 </macros>
314
315=== modified file 'lib/lp/bugs/templates/bugtarget-macros-filebug.pt'
316--- lib/lp/bugs/templates/bugtarget-macros-filebug.pt 2012-05-14 19:38:34 +0000
317+++ lib/lp/bugs/templates/bugtarget-macros-filebug.pt 2012-05-24 20:50:26 +0000
318@@ -330,11 +330,10 @@
319 <script type="text/javascript">
320 LPJS.use('lp.app.banner.privacy', function(Y) {
321 Y.on('domready', function() {
322- var cfg = {
323- notification_text: "This report will be private. "
324- + "You can disclose it later."
325- };
326- var banner = new Y.lp.app.banner.privacy.PrivacyBanner(cfg);
327+ var notification_text = "This report will be private. " +
328+ "You can disclose it later.";
329+ var banner = Y.lp.app.banner.privacy.getPrivacyBanner(
330+ notification_text);
331 banner.show();
332 });
333 });
334@@ -358,13 +357,14 @@
335 }, "input[name='field.information_type']");
336 };
337 var setup_security_related = function() {
338- var cfg = {
339- notification_text: "This report will be private "
340- + "because it is a security vulnerability. You "
341- + "can disclose it later."
342+ var notification_text: "This report will be private " +
343+ "because it is a security " +
344+ "vulnerability. You can " +
345+ "disclose it later.";
346 };
347 var privacy_ns = Y.lp.app.banner.privacy;
348- var banner = new privacy_ns.PrivacyBanner(cfg);
349+ var banner = privacy_ns.getPrivacyBanner(
350+ notification_text);
351 banner.render();
352 var sec = Y.one('[id="field.security_related"]');
353 sec.on('change', function() {