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

Revision history for this message
Adi Roiban (adiroiban) wrote :

Hi Edwin and thanks for the review!

I moved the try/catch in a single domready block, declared link as a local varibaled and used === to compare for null.

Here is the latests diff:
=== modified file 'lib/canonical/launchpad/javascript/translations/pofile.js'
--- lib/canonical/launchpad/javascript/translations/pofile.js 2010-03-22 14:59:40 +0000
+++ lib/canonical/launchpad/javascript/translations/pofile.js 2010-03-22 17:43:04 +0000
@@ -59,7 +59,7 @@

 self.updateNotificationBox = function(e) {
   var notice = Y.one('.important-notice-container');
- if (notice == null) {
+ 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;
@@ -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 == null) {
+ if (cancel_button === null) {
     // No cancel button was found to attach the action.
     return;
   }
@@ -197,6 +197,7 @@
 var initializeGlobalKeyBindings = function(fields) {

     Y.get('document').on("keyup", function(e) {
+ var link;
         // Shift+Alt+s - Save form
         if (e.shiftKey && e.altKey && e.keyCode == 83) {
             Y.one('#save_and_continue_button').invoke('click');

=== modified file 'lib/lp/translations/templates/pofile-translate.pt'
--- lib/lp/translations/templates/pofile-translate.pt 2010-03-22 14:59:44 +0000
+++ lib/lp/translations/templates/pofile-translate.pt 2010-03-22 17:34:27 +0000
@@ -25,25 +25,19 @@
             } catch (e) {
               Y.log(e, "error");
             }
- });

- Y.on('domready', function(e) {
             try {
               Y.lp.pofile.setupSuggestionDismissal();
             } catch (e) {
               Y.log(e, "error");
             }
- });

- Y.on('domready', function(e) {
             try {
               Y.lp.pofile.initializeKeyBindings();
             } catch (e) {
               Y.log(e, "error");
             }
- });

- Y.on('domready', function(e) {
             try {
               Y.lp.pofile.setFocus(autofocus_field);
             } catch (e) {

=== modified file 'lib/lp/translations/templates/translationmessage-translate.pt'
--- lib/lp/translations/templates/translationmessage-translate.pt 2010-03-22 14:50:08 +0000
+++ lib/lp/translations/templates/translationmessage-translate.pt 2010-03-22 17:35:02 +0000
@@ -21,17 +21,13 @@
             } catch (e) {
               Y.log(e, "error");
             }
- });

- Y.on('domready', function(e) {
             try {
               Y.lp.pofile.initializeKeyBindings();
             } catch (e) {
               Y.log(e, "error");
             }
- });

- Y.on('domready', function(e) {
             try {
               Y.lp.pofile.setFocus(autofocus_field);
             } catch (e) {

« Back to merge proposal