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

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.

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

-Edwin

>=== modified file 'lib/canonical/launchpad/javascript/translations/pofile.js'
>--- lib/canonical/launchpad/javascript/translations/pofile.js 2010-02-25 12:56:45 +0000
>+++ lib/canonical/launchpad/javascript/translations/pofile.js 2010-03-17 17:36:08 +0000
>@@ -55,10 +55,15 @@
> node.setStyle('display', 'none');
> });
> hide_anim.run();
>-}
>+};
>
> self.updateNotificationBox = function(e) {
> 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)

>+ // We have no notice container on this page, this is why there is
>+ // nothing more to be done by this function.
>+ return;
>+ }
> var balloon = notice.one('.important-notice-balloon');
> var dismiss_notice_cookie = ('translation-docs-for-' +
> documentation_cookie);
>@@ -74,6 +79,10 @@
> '.important-notice-cancel-button');
> // Cancel button starts out hidden. If user has JavaScript,
> // then we want to show it.
>+ if (!cancel_button) {

Same here.

>+ // No cancel button was found to attach the action.
>+ return;
>+ }
> cancel_button.setStyle('visibility', 'visible');
> cancel_button.on('click', function(e) {
> e.halt();
>@@ -202,30 +211,34 @@
> }
> // Shift+Alt+n - Go to next page in batch
> if (e.shiftKey && e.altKey && e.keyCode == 78) {
>- if (link = Y.one('#batchnav_next')){
>- window.location.assign(link.get('href'))
>+ link = Y.one('#batchnav_next');
>+ if (link){

Same for these if-statements.

>+ window.location.assign(link.get('href'));
> }
> }
> // Shift+Alt+p - Go to previous page in batch
> if (e.shiftKey && e.altKey && e.keyCode == 80) {
>- if (link = Y.one('#batchnav_previous')){
>- window.location.assign(link.get('href'))
>+ link = Y.one('#batchnav_previous');
>+ if (link){
>+ window.location.assign(link.get('href'));
> }
> }
> // Shift+Alt+a - Go to first page in batch
> if (e.shiftKey && e.altKey && e.keyCode == 65) {
>- if (link = Y.one('#batchnav_first')){
>- window.location.assign(link.get('href'))
>+ link = Y.one('#batchnav_first');
>+ if (link){
>+ window.location.assign(link.get('href'));
> }
> }
> // Shift+Alt+l - Go to last page in batch
> if (e.shiftKey && e.altKey && e.keyCode == 76) {
>- if (link = Y.one('#batchnav_last')){
>- window.location.assign(link.get('href'))
>+ link = Y.one('#batchnav_last');
>+ if (link){
>+ window.location.assign(link.get('href'));
> }
> }
> });

Please replace all the tab characters with spaces.
You can make vim display tab characters differently than spaces
by adding the following config to your .vimrc file.
    au BufEnter *.py set list listchars=tab:>*,trail:*
I have no idea how to do that in emacs.

>-}
>+};
>
>
> var initializeSuggestionsKeyBindings = function(stem) {
>@@ -243,7 +256,7 @@
> Y, node.get('id'));
> }
> });
>-}
>+};
>
>
> var initializeFieldsKeyBindings = function (fields) {
>@@ -321,7 +334,7 @@
> singular_copy_text);
> }
> }
>-}
>+};
>
>
> /**
>
>=== modified file 'lib/lp/translations/templates/pofile-translate.pt'
>--- lib/lp/translations/templates/pofile-translate.pt 2010-02-25 00:47:17 +0000
>+++ lib/lp/translations/templates/pofile-translate.pt 2010-03-17 17:36:08 +0000
>@@ -22,13 +22,21 @@
> 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);
>- 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.

> Y.on('domready', Y.lp.pofile.initializeKeyBindings);
> Y.on('domready', function(e) {
> Y.lp.pofile.setFocus(autofocus_field);
> });
> });
>+
>+
> </script>
> </div>
> <div metal:fill-slot="main">
>