launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #08192
[Merge] lp:~jcsackett/launchpad/simplify-everything into lp:launchpad
j.c.sackett has proposed merging lp:~jcsackett/launchpad/simplify-everything into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/simplify-everything/+merge/107240
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.
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.
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.
--
https://code.launchpad.net/~jcsackett/launchpad/simplify-everything/+merge/107240
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/simplify-everything into lp:launchpad.
=== modified file 'lib/lp/app/javascript/banners/banner.js'
--- lib/lp/app/javascript/banners/banner.js 2012-05-18 20:10:14 +0000
+++ lib/lp/app/javascript/banners/banner.js 2012-05-24 16:26:40 +0000
@@ -93,30 +93,15 @@
login_space.run();
},
- _createBanner: function () {
+ renderUI: function () {
var banner_data = {
badge: this.get('banner_icon'),
text: this.get('notification_text'),
- banner_id: this.get('banner_id')
};
- return Y.lp.mustache.to_html(
+ var banner_html = Y.lp.mustache.to_html(
this.get('banner_template'),
banner_data);
- },
-
- _updateBanner: function (banner_node) {
- text_node = banner_node.one('.banner-text');
- text_node.set('innerText', this.get('notification_text'));
- },
-
- renderUI: function() {
- var existing_banner = Y.one('#' + this.get('banner_id'));
- if (existing_banner) {
- this._updateBanner(existing_banner);
- } else {
- var banner_html = this._createBanner();
- this.get('contentBox').append(banner_html);
- }
+ this.get('contentBox').append(banner_html);
},
bindUI: function() {
@@ -127,17 +112,24 @@
this._hideBanner();
}
});
- }
+ },
+
+ updateText: function (new_text) {
+ if (!Y.Lang.isValue(new_text)) {
+ new_text = this.get('notification_text');
+ }
+ text_node = this.get('contentBox').one('.banner-text');
+ text_node.set('innerText', new_text);
+ },
+
}, {
ATTRS: {
- banner_id: { value: "base-banner" },
notification_text: { value: "" },
banner_icon: { value: "<span></span>" },
banner_template: {
valueFn: function() {
return [
- '<div id="{{ banner_id }}"',
- 'class="global-notification transparent hidden">',
+ '<div class="global-notification transparent hidden">',
'{{{ badge }}}',
'<span class="banner-text">{{ text }}</span>',
"</div>"].join('')
=== modified file 'lib/lp/app/javascript/banners/beta-notification.js'
--- lib/lp/app/javascript/banners/beta-notification.js 2012-05-18 22:26:44 +0000
+++ lib/lp/app/javascript/banners/beta-notification.js 2012-05-24 16:26:40 +0000
@@ -5,20 +5,16 @@
ns.BetaBanner = Y.Base.create('betaBanner', baseBanner, [], {
- _createBanner: function () {
+ renderUI: function () {
var banner_data = {
badge: this.get('banner_icon'),
text: this.get('notification_text'),
features: this.get('features'),
- banner_id: this.get('banner_id')
};
- return Y.lp.mustache.to_html(
+ var banner_html = Y.lp.mustache.to_html(
this.get('banner_template'),
banner_data);
- },
-
- renderUI: function () {
- baseBanner.prototype.renderUI.apply(this, arguments);
+ this.get('contentBox').append(banner_html);
var beta_node = Y.one('.global-notification');
var close_box = Y.Node.create(
'<a href="#" class="global-notification-close">Hide' +
@@ -38,14 +34,12 @@
}, {
ATTRS: {
- banner_id: { value: "beta-banner" },
notification_text: { value: "Some parts of this page are in beta: " },
banner_icon: { value: '<span class="beta-warning">BETA!</span>' },
banner_template: {
valueFn: function() {
return [
- '<div id="{{ banner_id }}"',
- 'class="global-notification transparent hidden">',
+ '<div class="global-notification transparent hidden">',
'{{{ badge }}}',
'<span class="banner-text">',
'{{ text }}{{{ features }}}',
@@ -72,11 +66,17 @@
}
});
+// For the beta banner to work, it needs to have one instance, and one
+// instance only.
+_singleton_beta_banner = null;
ns.show_beta_if_needed = function () {
- var banner = new ns.BetaBanner();
- if (banner.get('features').length !== 0) {
- banner.render();
- banner.show();
+ if (_singleton_beta_banner === null) {
+ var src = Y.one('.yui3-betabanner')
+ _singleton_beta_banner = new ns.BetaBanner({ srcNode: src });
+ }
+ if (_singleton_beta_banner.get('features').length !== 0) {
+ _singleton_beta_banner.render();
+ _singleton_beta_banner.show();
}
}
=== modified file 'lib/lp/app/javascript/banners/privacy.js'
--- lib/lp/app/javascript/banners/privacy.js 2012-05-17 21:04:18 +0000
+++ lib/lp/app/javascript/banners/privacy.js 2012-05-24 16:26:40 +0000
@@ -5,7 +5,6 @@
ns.PrivacyBanner = Y.Base.create('privacyBanner', baseBanner, [], {}, {
ATTRS: {
- banner_id: { value: "privacy-banner" },
notification_text: {
value: "The information on this page is private."
},
@@ -18,12 +17,16 @@
// For the privacy banner to work, it needs to have one instance, and one
// instance only.
var _singleton_privacy_banner = null;
-
-ns.getPrivacyBanner = function () {
+ns.getPrivacyBanner = function (banner_text) {
+
if (_singleton_privacy_banner === null) {
- _singleton_privacy_banner = new ns.PrivacyBanner();
+ var src = Y.one('.yui3-privacybanner')
+ _singleton_privacy_banner = new ns.PrivacyBanner({ srcNode: src });
_singleton_privacy_banner.render();
}
+ if (Y.Lang.isValue(banner_text)) {
+ _singleton_privacy_banner.updateText(banner_text);
+ }
return _singleton_privacy_banner;
}
=== modified file 'lib/lp/app/javascript/banners/tests/test_banner.js'
--- lib/lp/app/javascript/banners/tests/test_banner.js 2012-05-22 21:31:04 +0000
+++ lib/lp/app/javascript/banners/tests/test_banner.js 2012-05-24 16:26:40 +0000
@@ -62,7 +62,6 @@
banner.render();
var banner_node = Y.one(".global-notification");
- console.log(banner_node);
var badge = banner_node.one('.sprite');
Y.Assert.isObject(banner_node);
Y.Assert.areEqual(cfg.notification_text, banner_node.get('text'));
@@ -72,8 +71,7 @@
test_show: function() {
var banner = new Y.lp.app.banner.Banner({ skip_animation: true });
banner.render();
- banner.show();
-
+ banner.show();
var banner_node = Y.one(".global-notification");
Y.Assert.isFalse(banner_node.hasClass('hidden'));
},
@@ -94,25 +92,18 @@
this.wait(check, wait_for_anim);
},
- test_only_one_banner: function () {
- // There can only be one banner node for a given type of banner.
+ test_updateText: function() {
var banner = new Y.lp.app.banner.Banner({ skip_animation: true });
banner.render();
- banner.show();
- Y.Assert.areEqual(1, Y.all('.global-notification').size());
+ var new_text = 'some new text';
+ banner.updateText(new_text);
+ var banner_node = Y.one(".global-notification");
+ Y.Assert.areEqual(new_text, banner_node.get('text'));
- var new_text = 'This is new text';
- var banner2 = new Y.lp.app.banner.Banner({
- notification_text: new_text,
- skip_animation: true
- });
- banner2.render();
- banner2.show();
- Y.Assert.areEqual(1, Y.all('.global-notification').size());
- var banner_node = Y.one('.global-notification');
- Y.Assert.areEqual(
- new_text,
- Y.one('.global-notification').get('text'));
+ banner.updateText();
+ var banner_node = Y.one(".global-notification");
+ Y.Assert.areEqual("", banner_node.get('text'));
}
}));
+
}, '0.1', {'requires': ['test', 'console', 'lp.app.banner']});
=== modified file 'lib/lp/app/javascript/banners/tests/test_privacy.js'
--- lib/lp/app/javascript/banners/tests/test_privacy.js 2012-05-22 21:31:04 +0000
+++ lib/lp/app/javascript/banners/tests/test_privacy.js 2012-05-24 16:26:40 +0000
@@ -36,6 +36,20 @@
'<span class="sprite notification-private"></span>',
banner.get('banner_icon'));
},
+
+ test_only_one_banner: function () {
+ // getPrivacyBanner only returns one banner.
+ var banner = Y.lp.app.banner.privacy.getPrivacyBanner();
+ Y.Assert.areEqual(1, Y.all('.global-notification').size());
+
+ var new_text = 'This is new text';
+ var banner = Y.lp.app.banner.privacy.getPrivacyBanner(new_text);
+ Y.Assert.areEqual(1, Y.all('.global-notification').size());
+ var banner_node = Y.one('.global-notification');
+ Y.Assert.areEqual(
+ new_text,
+ Y.one('.global-notification').get('text'));
+ }
}));
}, '0.1', {'requires': ['test', 'console', 'lp.app.banner.privacy']});
=== modified file 'lib/lp/app/templates/banner-macros.pt'
--- lib/lp/app/templates/banner-macros.pt 2012-05-21 20:12:08 +0000
+++ lib/lp/app/templates/banner-macros.pt 2012-05-24 16:26:40 +0000
@@ -9,35 +9,39 @@
<metal:privacy define-macro="privacy-banner">
<tal:show-banner condition="view/private">
- <div class="yui3-privacybanner-content">
- <div id="privacy-banner" class="global-notification">
- <span class="sprite notification-private"></span>
- <span class="banner-text">The information on this page is private.</span>
+ <div class="yui3-widget yui3-banner yui3-privacybanner">
+ <div class="yui3-privacybanner-content">
+ <div class="global-notification">
+ <span class="sprite notification-private"></span>
+ <span class="banner-text">The information on this page is private.</span>
+ </div>
</div>
</div>
</tal:show-banner>
</metal:privacy>
<metal:beta define-macro="beta-banner">
-<tal:show-banner condition="view/beta_features">
-<div class="yui3-betabanner-content">
- <div id="beta-banner" class="global-notification">
- <span class="beta-warning">BETA!</span>
- <span class="banner-text">
- Some parts of this page are in beta:
- <span class="beta-feature">
- <tal:features
- repeat="feature view/beta_features">
- <tal:feature replace="feature/title" />
- <tal:link condition="feature/url">
- (<a tal:attributes="href feature/url" class="info-link">read more</a>)
- </tal:link>
- </tal:features>
- <span>
- </span>
- </div>
-</div>
-</tal:show-banner>
+ <tal:show-banner condition="view/beta_features">
+ <div class="yui3-widget yui3-banner yui3-betabanner">
+ <div class="yui3-betabanner-content">
+ <div class="global-notification">
+ <span class="beta-warning">BETA!</span>
+ <span class="banner-text">
+ Some parts of this page are in beta:
+ <span class="beta-feature">
+ <tal:features
+ repeat="feature view/beta_features">
+ <tal:feature replace="feature/title" />
+ <tal:link condition="feature/url">
+ (<a tal:attributes="href feature/url" class="info-link">read more</a>)
+ </tal:link>
+ </tal:features>
+ <span>
+ </span>
+ </div>
+ </div>
+ </div>
+ </tal:show-banner>
</metal:beta>
</macros>
=== modified file 'lib/lp/bugs/templates/bugtarget-macros-filebug.pt'
--- lib/lp/bugs/templates/bugtarget-macros-filebug.pt 2012-05-14 19:38:34 +0000
+++ lib/lp/bugs/templates/bugtarget-macros-filebug.pt 2012-05-24 16:26:40 +0000
@@ -330,11 +330,10 @@
<script type="text/javascript">
LPJS.use('lp.app.banner.privacy', function(Y) {
Y.on('domready', function() {
- var cfg = {
- notification_text: "This report will be private. "
- + "You can disclose it later."
- };
- var banner = new Y.lp.app.banner.privacy.PrivacyBanner(cfg);
+ var notification_text = "This report will be private. " +
+ "You can disclose it later.";
+ var banner = Y.lp.app.banner.privacy.getPrivacyBanner(
+ notification_text);
banner.show();
});
});
@@ -358,13 +357,14 @@
}, "input[name='field.information_type']");
};
var setup_security_related = function() {
- var cfg = {
- notification_text: "This report will be private "
- + "because it is a security vulnerability. You "
- + "can disclose it later."
+ var notification_text: "This report will be private " +
+ "because it is a security " +
+ "vulnerability. You can " +
+ "disclose it later.";
};
var privacy_ns = Y.lp.app.banner.privacy;
- var banner = new privacy_ns.PrivacyBanner(cfg);
+ var banner = privacy_ns.getPrivacyBanner(
+ notification_text);
banner.render();
var sec = Y.one('[id="field.security_related"]');
sec.on('change', function() {
Follow ups
