Code review comment for lp:~adiroiban/launchpad/bug-540105

Hi,
> Hi Adi,
>
> This is a good improvement. I ran into an annoying problem running the tests.
> The tests use the combined javascript file, but "make jsbuild" was not getting
> run automatically. I also noticed that translationmessage-translate.pt and
> pofile-translate.pt load pofile.js directly, when the <script> tag really
> should be added to lib/lp/app/templates/base-layout-macros.pt next to all
> the other js files in the ${lp_js} directory.

I have moved the pofile.js inclusing in base-layout-macros.pt.

> Since I was confused by some of your changes that I don't think were
> necessary, I'm marking this
> needs-fixing

Thanks for the review

[snip]

> > var notice = Y.one('.important-notice-container');
> >+ if (!notice) {
>
>
> Since Y.one() is expected to return null when the element is not found,
> the preferred if-statement is:
> if (notice !== null)
Thanks for this remark.

I have changed the if statement to explicitly test agains null.

Same for the other cases.

[snip]
> >+ LPS.use('lp.pofile', function(Y) {
> > Y.on('domready', Y.lp.pofile.setupSuggestionDismissal);
> >- Y.on('domready', Y.lp.pofile.updateNotificationBox);
> >+ });
> >+
> >+ LPS.use('lp.pofile', function(Y) {
>
>
> Why are you calling LPS.use('lp.pofile', ...) repeatedly? This
> should have no effect unless there is something seriously wrong with
> lp.pofile.
I moved them back in the same sandbox.
I split independent functions in different sandboxes so that the failure of one function to not affect the others.

Here is the latest diff:

=== modified file 'lib/canonical/launchpad/javascript/translations/pofile.js'
--- lib/canonical/launchpad/javascript/translations/pofile.js 2010-03-17 13:11:23 +0000
+++ lib/canonical/launchpad/javascript/translations/pofile.js 2010-03-17 22:18:24 +0000
@@ -59,7 +59,7 @@

 self.updateNotificationBox = function(e) {
   var notice = Y.one('.important-notice-container');
- if (!notice) {
+ if (notice == null) {
     // We have no notice container on this page, this is why there is
     // nothing more to be done by this function.
     return;
@@ -71,7 +71,7 @@
   // Check the cookie to see if the user has already dismissed
   // the notification box for this session.
   var already_seen = Y.Cookie.get(dismiss_notice_cookie, Boolean);
- if (already_seen) {
+ if (already_seen !== null) {
      notice.setStyle('display', 'none');
   }

@@ -79,7 +79,7 @@
       '.important-notice-cancel-button');
   // Cancel button starts out hidden. If user has JavaScript,
   // then we want to show it.
- if (!cancel_button) {
+ if (cancel_button == null) {
     // No cancel button was found to attach the action.
     return;
   }
@@ -94,7 +94,7 @@

 self.setFocus = function(field) {
     // if there is nofield, do nothing
- if (Y.one('#' + field)) {
+ if (Y.one('#' + field) !== null) {
         Y.one('#' + field).focus();
     }
 };
@@ -151,7 +151,7 @@
         original_singular, translation_singular, singular_select);

     // Copy plural text if needed
- if (Y.one('#' + translation_plural + '1')) {
+ if (Y.one('#' + translation_plural + '1') !== null) {
         copyOriginalTextPlural(
             original_plural, translation_plural, plural_forms);
     }
@@ -162,7 +162,7 @@

 var selectWidgetByID = function(widget) {
     var node = Y.one('#' + widget);
- if (node) {
+ if (node !== null) {
         node.set('checked', true);
     }
 };
@@ -170,7 +170,7 @@

 var toggleWidget = function(widget) {
     var node = Y.one('#' + widget);
- if (node) {
+ if (node !== null) {
         if (node.get('checked')) {
             node.set('checked', false);
         } else {
@@ -196,48 +196,48 @@

 var initializeGlobalKeyBindings = function(fields) {

- Y.get('document').on("keyup", function(e) {
+ Y.get('document').on("keyup", function(e) {
         // Shift+Alt+s - Save form
- if (e.shiftKey && e.altKey && e.keyCode == 83) {
- Y.one('#save_and_continue_button').invoke('click');
- }
+ if (e.shiftKey && e.altKey && e.keyCode == 83) {
+ Y.one('#save_and_continue_button').invoke('click');
+ }
         // Shift+Alt+f - Go to search field
- if (e.shiftKey && e.altKey && e.keyCode == 70) {
- self.setFocus('search_box');
- }
+ if (e.shiftKey && e.altKey && e.keyCode == 70) {
+ self.setFocus('search_box');
+ }
         // Shift+Alt+b - Go to first translation field
- if (e.shiftKey && e.altKey && e.keyCode == 66) {
- self.setFocus(fields[0]);
- }
+ if (e.shiftKey && e.altKey && e.keyCode == 66) {
+ self.setFocus(fields[0]);
+ }
         // Shift+Alt+n - Go to next page in batch
- if (e.shiftKey && e.altKey && e.keyCode == 78) {
- link = Y.one('#batchnav_next');
- if (link){
- window.location.assign(link.get('href'));
- }
- }
+ if (e.shiftKey && e.altKey && e.keyCode == 78) {
+ link = Y.one('#batchnav_next');
+ if (link !== null){
+ window.location.assign(link.get('href'));
+ }
+ }
         // Shift+Alt+p - Go to previous page in batch
- if (e.shiftKey && e.altKey && e.keyCode == 80) {
- link = Y.one('#batchnav_previous');
- if (link){
- window.location.assign(link.get('href'));
- }
- }
+ if (e.shiftKey && e.altKey && e.keyCode == 80) {
+ link = Y.one('#batchnav_previous');
+ if (link !== null){
+ window.location.assign(link.get('href'));
+ }
+ }
         // Shift+Alt+a - Go to first page in batch
- if (e.shiftKey && e.altKey && e.keyCode == 65) {
- link = Y.one('#batchnav_first');
- if (link){
- window.location.assign(link.get('href'));
- }
- }
+ if (e.shiftKey && e.altKey && e.keyCode == 65) {
+ link = Y.one('#batchnav_first');
+ if (link !== null){
+ window.location.assign(link.get('href'));
+ }
+ }
         // Shift+Alt+l - Go to last page in batch
- if (e.shiftKey && e.altKey && e.keyCode == 76) {
- link = Y.one('#batchnav_last');
- if (link){
- window.location.assign(link.get('href'));
- }
- }
- });
+ if (e.shiftKey && e.altKey && e.keyCode == 76) {
+ link = Y.one('#batchnav_last');
+ if (link !== null){
+ window.location.assign(link.get('href'));
+ }
+ }
+ });
 };

=== modified file 'lib/lp/app/templates/base-layout-macros.pt'
--- lib/lp/app/templates/base-layout-macros.pt 2010-03-15 17:17:05 +0000
+++ lib/lp/app/templates/base-layout-macros.pt 2010-03-17 23:14:12 +0000
@@ -211,6 +211,9 @@
     <script type="text/javascript"
             tal:attributes="src string:${lp_js}/registry/team.js"></script>

+ <script type="text/javascript"
+ tal:attributes="src string:${lp_js}/translations/pofile.js"></script>
+
   </tal:devmode>
   <tal:production condition="not:devmode">
     <script type="text/javascript"

=== modified file 'lib/lp/translations/templates/pofile-translate.pt'
--- lib/lp/translations/templates/pofile-translate.pt 2010-03-17 12:00:59 +0000
+++ lib/lp/translations/templates/pofile-translate.pt 2010-03-17 22:12:53 +0000
@@ -14,22 +14,12 @@
     }
     </style>

- <script type="text/javascript"
- tal:condition="devmode"
- tal:define="lp_js string:${icingroot}/build"
- tal:attributes="src string:${lp_js}/translations/pofile.js"></script>
     <script type="text/javascript">
       registerLaunchpadFunction(insertAllExpansionButtons);

       LPS.use('lp.pofile', function(Y) {
           Y.on('domready', Y.lp.pofile.updateNotificationBox);
- });
-
- LPS.use('lp.pofile', function(Y) {
           Y.on('domready', Y.lp.pofile.setupSuggestionDismissal);
- });
-
- LPS.use('lp.pofile', function(Y) {
           Y.on('domready', Y.lp.pofile.initializeKeyBindings);
           Y.on('domready', function(e) {
             Y.lp.pofile.setFocus(autofocus_field);

=== modified file 'lib/lp/translations/templates/translationmessage-translate.pt'
--- lib/lp/translations/templates/translationmessage-translate.pt 2010-02-25 00:47:17 +0000
+++ lib/lp/translations/templates/translationmessage-translate.pt 2010-03-17 23:14:49 +0000
@@ -13,10 +13,6 @@
         color: lightgray;
     }
     </style>
- <script type="text/javascript"
- tal:condition="devmode"
- tal:define="lp_js string:${icingroot}/build"
- tal:attributes="src string:${lp_js}/translations/pofile.js"></script>
     <script type="text/javascript">
       LPS.use('node', 'lp.pofile', function(Y) {
           Y.on('domready', Y.lp.pofile.setupSuggestionDismissal);