launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04474
[Merge] lp:~jcsackett/launchpad/global-privacy-ribbon into lp:launchpad
j.c.sackett has proposed merging lp:~jcsackett/launchpad/global-privacy-ribbon into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jcsackett/launchpad/global-privacy-ribbon/+merge/70327
Summary
=======
After doing a lot of work to add the privacy notification code to various templates, it became clear that it's better to lose a bit of flexability in order to have the notification automatically displayed for any private context.
To do this, the privacy notification code had to be moved to the base layout javascript macros, and the privacy notification functions had to be altered a bit to deal with being less flexible in their invocation.
Pre-implementation notes
========================
Initially proposed by William Grant. There were subsequent discussions on what exactly the tradeoffs would be with Curtis Hovey.
Implementation details
======================
lib/lp/app/javascript/privacy.js
lib/lp/app/templates/base-layout-macros.pt
------------------------------------------
The base layout macros had the setup and display code added within a privacy feature flag. The javascript file was modified to remove the variable indicating whether or not the notification was enabled, as it's only in place at all if the feature flag is set. Some configuration was also removed, as the portlet-highlighting behavior is now automatic if a privacy portlet exists.
lib/lp/bugs/templates/bugtask-index.pt
lib/lp/code/templates/branch-index.pt
lib/lp/code/templates/branchmergeproposal-index.pt
lib/lp/bugs/templates/bugcomment-index.pt
-----------------------------------------
The templates have all had the individual setup and display calls removed.
Tests
=====
firefox lib/lp/app/javascript/tests/test_privacy.js
QA
==
Check all the various private contexts on Launchpad; with the flag set, they should be guarded by the new privacy notification. Without the flag, the ne new notification shouldn't appear. On public contexts, the notification should not display.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/code/templates/branchmergeproposal-index.pt
lib/lp/bugs/templates/bugcomment-index.pt
lib/lp/app/javascript/privacy.js
lib/lp/app/templates/base-layout-macros.pt
lib/lp/bugs/templates/bugtask-index.pt
lib/lp/code/templates/branch-index.pt
./lib/lp/app/javascript/privacy.js
134: Expected '!==' and instead saw '!='.
Lint will be cleaned before landing.
--
https://code.launchpad.net/~jcsackett/launchpad/global-privacy-ribbon/+merge/70327
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jcsackett/launchpad/global-privacy-ribbon into lp:launchpad.
=== modified file 'lib/lp/app/javascript/privacy.js'
--- lib/lp/app/javascript/privacy.js 2011-07-29 22:27:13 +0000
+++ lib/lp/app/javascript/privacy.js 2011-08-03 16:25:08 +0000
@@ -50,55 +50,48 @@
}
namespace.setup_privacy_notification = setup_privacy_notification;
-function display_privacy_notification(highlight_portlet_on_close) {
- /* Check if the feature flag is set for this notification. */
- var highlight = true;
- if (highlight_portlet_on_close !== undefined) {
- highlight = highlight_portlet_on_close;
- }
- if (privacy_notification_enabled) {
- /* Set a temporary class on the body for the feature flag,
- this is because we have no way to use feature flags in
- css directly. This should be removed if the feature
- is accepted. */
- var body = Y.one('body');
- body.addClass('feature-flag-bugs-private-notification-enabled');
- /* Set the visible flag so that the content moves down. */
- body.addClass('global-notification-visible');
-
- var global_notification = Y.one('.global-notification');
- if (global_notification.hasClass('hidden')) {
- global_notification.addClass('transparent');
- global_notification.removeClass('hidden');
-
- var fade_in = new Y.Anim({
- node: global_notification,
- to: {opacity: 1},
- duration: 0.3
- });
- var body_space = new Y.Anim({
- node: 'body',
- to: {'paddingTop': '40px'},
- duration: 0.2,
- easing: Y.Easing.easeOut
- });
- var login_space = new Y.Anim({
- node: '.login-logout',
- to: {'top': '45px'},
- duration: 0.2,
- easing: Y.Easing.easeOut
- });
-
- fade_in.run();
- body_space.run();
- login_space.run();
- }
-
- Y.one('.global-notification-close').on('click', function(e) {
- hide_privacy_notification(highlight);
- e.halt();
- });
- }
+function display_privacy_notification() {
+ /* Set a temporary class on the body for the feature flag,
+ this is because we have no way to use feature flags in
+ css directly. This should be removed if the feature
+ is accepted. */
+ var body = Y.one('body');
+ body.addClass('feature-flag-bugs-private-notification-enabled');
+ /* Set the visible flag so that the content moves down. */
+ body.addClass('global-notification-visible');
+
+ var global_notification = Y.one('.global-notification');
+ if (global_notification.hasClass('hidden')) {
+ global_notification.addClass('transparent');
+ global_notification.removeClass('hidden');
+
+ var fade_in = new Y.Anim({
+ node: global_notification,
+ to: {opacity: 1},
+ duration: 0.3
+ });
+ var body_space = new Y.Anim({
+ node: 'body',
+ to: {'paddingTop': '40px'},
+ duration: 0.2,
+ easing: Y.Easing.easeOut
+ });
+ var login_space = new Y.Anim({
+ node: '.login-logout',
+ to: {'top': '45px'},
+ duration: 0.2,
+ easing: Y.Easing.easeOut
+ });
+
+ fade_in.run();
+ body_space.run();
+ login_space.run();
+ }
+
+ Y.one('.global-notification-close').on('click', function(e) {
+ hide_privacy_notification();
+ e.halt();
+ });
}
namespace.display_privacy_notification = display_privacy_notification;
@@ -107,48 +100,47 @@
*
* This should be called after the page has loaded e.g. on 'domready'.
*/
-function hide_privacy_notification(highlight_portlet) {
- if (privacy_notification_enabled) {
- if (!Y.one('.global-notification').hasClass('hidden')) {
- var fade_out = new Y.Anim({
- node: '.global-notification',
- to: {opacity: 0},
- duration: 0.3
- });
- var body_space = new Y.Anim({
- node: 'body',
- to: {'paddingTop': 0},
- duration: 0.2,
- easing: Y.Easing.easeOut
- });
- var login_space = new Y.Anim({
- node: '.login-logout',
- to: {'top': '6px'},
- duration: 0.2,
- easing: Y.Easing.easeOut
- });
- fade_out.on('end', function() {
- fade_out.get('node').addClass('hidden');
- });
- body_space.on('end', function() {
- Y.one('body').removeClass('global-notification-visible');
- });
-
- fade_out.run();
- body_space.run();
- login_space.run();
-
- if (highlight_portlet) {
- var portlet_colour = new Y.Anim({
- node: '.portlet.private',
- to: {
- color: '#fff',
- backgroundColor:'#8d1f1f'
- },
- duration: 0.4
- });
- portlet_colour.run();
- }
+function hide_privacy_notification() {
+ if (!Y.one('.global-notification').hasClass('hidden')) {
+ var fade_out = new Y.Anim({
+ node: '.global-notification',
+ to: {opacity: 0},
+ duration: 0.3
+ });
+ var body_space = new Y.Anim({
+ node: 'body',
+ to: {'paddingTop': 0},
+ duration: 0.2,
+ easing: Y.Easing.easeOut
+ });
+ var login_space = new Y.Anim({
+ node: '.login-logout',
+ to: {'top': '6px'},
+ duration: 0.2,
+ easing: Y.Easing.easeOut
+ });
+ fade_out.on('end', function() {
+ fade_out.get('node').addClass('hidden');
+ });
+ body_space.on('end', function() {
+ Y.one('body').removeClass('global-notification-visible');
+ });
+
+ fade_out.run();
+ body_space.run();
+ login_space.run();
+
+ var privacy_portlet = Y.one('.portlet.private');
+ if (privacy_portlet != null) {
+ var portlet_colour = new Y.Anim({
+ node: privacy_portlet,
+ to: {
+ color: '#fff',
+ backgroundColor:'#8d1f1f'
+ },
+ duration: 0.4
+ });
+ portlet_colour.run();
}
}
}
=== modified file 'lib/lp/app/templates/base-layout-macros.pt'
--- lib/lp/app/templates/base-layout-macros.pt 2011-07-29 14:32:45 +0000
+++ lib/lp/app/templates/base-layout-macros.pt 2011-08-03 16:25:08 +0000
@@ -106,16 +106,21 @@
<metal:load-lavascript use-macro="context/@@+base-layout-macros/load-javascript" />
- <tal:if condition="request/features/bugs.private_notification.enabled">
- <script type="text/javascript">
- var privacy_notification_enabled = true;
- </script>
- </tal:if>
- <tal:if condition="not:request/features/bugs.private_notification.enabled">
- <script type="text/javascript">
- var privacy_notification_enabled = false;
- </script>
- </tal:if>
+ <tal:if condition="request/features/bugs.private_notification.enabled">
+ <script type="text/javascript">
+ LPS.use('base', 'node', 'oop', 'event', 'lp.bugs.bugtask_index',
+ 'lp.bugs.subscribers',
+ 'lp.code.branchmergeproposal.diff', 'lp.comments.hide',
+ function(Y) {
+ Y.on("domready", function () {
+ if (Y.one(document.body).hasClass('private')) {
+ Y.lp.app.privacy.setup_privacy_notification();
+ Y.lp.app.privacy.display_privacy_notification();
+ }
+ });
+ });
+ </script>
+ </tal:if>
<script id="base-layout-load-scripts" type="text/javascript">
LPS.use('node', 'event-delegate', 'lp', 'lp.app.links', 'lp.app.longpoll', function(Y) {
=== modified file 'lib/lp/bugs/templates/bugcomment-index.pt'
--- lib/lp/bugs/templates/bugcomment-index.pt 2011-07-29 21:53:43 +0000
+++ lib/lp/bugs/templates/bugcomment-index.pt 2011-08-03 16:25:08 +0000
@@ -8,22 +8,6 @@
>
<body>
<metal:block fill-slot="head_epilogue">
- <tal:if condition="request/features/bugs.private_notification.enabled">
- <script>
- LPS.use('base', 'node', 'lp.app.privacy', function(Y) {
- Y.on("domready", function () {
- if (Y.one(document.body).hasClass('private')) {
- var config = {
- notification_text: 'This comment is on a private bug',
- hidden: true
- };
- Y.lp.app.privacy.setup_privacy_notification(config);
- Y.lp.app.privacy.display_privacy_notification(false);
- }
- });
- });
- </script>
- </tal:if>
</metal:block>
<div metal:fill-slot="main" tal:define="comment view/comment">
<h1 tal:content="view/page_title">Foo doesn't work</h1>
=== modified file 'lib/lp/bugs/templates/bugtask-index.pt'
--- lib/lp/bugs/templates/bugtask-index.pt 2011-07-29 20:17:56 +0000
+++ lib/lp/bugs/templates/bugtask-index.pt 2011-08-03 16:25:08 +0000
@@ -34,28 +34,6 @@
});
});
</script>
- <tal:if condition="request/features/bugs.private_notification.enabled">
- <script type="text/javascript">
- LPS.use('base', 'node', 'oop', 'event', 'lp.bugs.bugtask_index',
- 'lp.bugs.subscribers',
- 'lp.code.branchmergeproposal.diff', 'lp.comments.hide',
- function(Y) {
- /*
- * Display the privacy notification if the bug is private
- */
- Y.on("domready", function () {
- if (Y.one(document.body).hasClass('private')) {
- var config = {
- notification_text: 'This bug is private',
- hidden: true
- };
- Y.lp.app.privacy.setup_privacy_notification(config);
- Y.lp.app.privacy.display_privacy_notification();
- }
- });
- });
- </script>
- </tal:if>
<style type="text/css">
/* Align the 'add comment' link to the right of the comment box. */
#add-comment-form textarea { width: 100%; }
=== modified file 'lib/lp/code/templates/branch-index.pt'
--- lib/lp/code/templates/branch-index.pt 2011-08-01 16:05:59 +0000
+++ lib/lp/code/templates/branch-index.pt 2011-08-03 16:25:08 +0000
@@ -51,23 +51,6 @@
});
"/>
- <tal:if condition="request/features/bugs.private_notification.enabled">
- <script>
- LPS.use('base', 'node', 'lp.app.privacy', function(Y) {
- Y.on("domready", function () {
- if (Y.one(document.body).hasClass('private')) {
- var config = {
- notification_text: 'This is a private branch',
- hidden: true
- };
- Y.lp.app.privacy.setup_privacy_notification(config);
- Y.lp.app.privacy.display_privacy_notification(true);
- }
- });
- });
- </script>
- </tal:if>
-
</metal:block>
<body>
=== modified file 'lib/lp/code/templates/branchmergeproposal-index.pt'
--- lib/lp/code/templates/branchmergeproposal-index.pt 2011-08-01 18:51:18 +0000
+++ lib/lp/code/templates/branchmergeproposal-index.pt 2011-08-03 16:25:08 +0000
@@ -48,22 +48,6 @@
padding-bottom: 10px;
}
</style>
- <tal:if condition="request/features/bugs.private_notification.enabled">
- <script>
- LPS.use('base', 'node', 'lp.app.privacy', function(Y) {
- Y.on("domready", function () {
- if (Y.one(document.body).hasClass('private')) {
- var config = {
- notification_text: 'This merge proposal is for a private branch',
- hidden: true
- };
- Y.lp.app.privacy.setup_privacy_notification(config);
- Y.lp.app.privacy.display_privacy_notification(false);
- }
- });
- });
- </script>
- </tal:if>
</metal:block>
<tal:registering metal:fill-slot="registering">
