Merge lp:~adiroiban/launchpad/bug-540105 into lp:launchpad
Status: | Merged |
---|---|
Approved by: | Edwin Grubbs |
Approved revision: | no longer in the source branch. |
Merged at revision: | not available |
Proposed branch: | lp:~adiroiban/launchpad/bug-540105 |
Merge into: | lp:launchpad |
Diff against target: |
338 lines (+152/-55) 5 files modified
lib/canonical/launchpad/javascript/translations/pofile.js (+54/-40) lib/lp/app/templates/base-layout-macros.pt (+3/-0) lib/lp/translations/templates/pofile-translate.pt (+26/-8) lib/lp/translations/templates/translationmessage-translate.pt (+17/-7) lib/lp/translations/windmill/tests/test_pofile_translate.py (+52/-0) |
To merge this branch: | bzr merge lp:~adiroiban/launchpad/bug-540105 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Edwin Grubbs (community) | code | Approve | |
Review via email: mp+21552@code.launchpad.net |
Commit message
Move independent functionality in different JS sandboxes and fix
pofile.js updateNotificat
Description of the change
= Bug 540105 =
I noticed this behaviour in edge:
When entering text in the input box for a translation, this textbox does no longer get the focus by automatically selecting the checkbox dot on its left.
This leads to translators not noticing it, pressing "Save" and loosing all their translations.
== Proposed fix ==
Move independent functionality in different JS sandboxes and fix
pofile.js updateNotificat
== Pre-implementation notes ==
This branch is a hotfix for this problem.
I have opened bug 540216 and working on improving the general test coverage for pofile.js code.
== Implementation details ==
I have also fixes some other lint warnings.
== Tests ==
lp-test -t pofile_
== Demo and Q/A ==
Login as admin and go to:
https:/
In the "New translation" field, enter some text. The radio button before the text field should be now selected.
= Launchpad lint =
Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.
Linting changed files:
lib/canonical
== JSLint notices ==
No handlers could be found for logger "bzr"
jslint: Lint found in '/home/
Line 303 character 18: Be careful when making functions within a loop. Consider putting the function in a closure.
},
Line 310 character 18: Be careful when making functions within a loop. Consider putting the function in a closure.
}, '#' + fields[key], 'down:68+
Line 316 character 18: Be careful when making functions within a loop. Consider putting the function in a closure.
}, '#' + fields[key], 'down:48+
jslint: 1 file to lint.
Hi Adi,
This is a good improvement. I ran into an annoying problem running the tests. age-translate. pt and app/templates/ base-layout- macros. pt next to all
The tests use the combined javascript file, but "make jsbuild" was not getting
run automatically. I also noticed that translationmess
pofile-translate.pt load pofile.js directly, when the <script> tag really
should be added to lib/lp/
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' launchpad/ javascript/ translations/ pofile. js 2010-02-25 12:56:45 +0000 launchpad/ javascript/ translations/ pofile. js 2010-03-17 17:36:08 +0000 'display' , 'none'); ficationBox = function(e) { .important- notice- container' );
>--- lib/canonical/
>+++ lib/canonical/
>@@ -55,10 +55,15 @@
> node.setStyle(
> });
> hide_anim.run();
>-}
>+};
>
> self.updateNoti
> var notice = Y.one('
>+ 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 one('.important -notice- balloon' ); notice_ cookie = ('translation- docs-for- ' + cookie) ; notice- cancel- button' );
>+ // nothing more to be done by this function.
>+ return;
>+ }
> var balloon = notice.
> var dismiss_
> documentation_
>@@ -74,6 +79,10 @@
> '.important-
> // 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. button. setStyle( 'visibility' , 'visible'); button. on('click' , function(e) { #batchnav_ next')) { location. assign( link.get( 'href') ) #batchnav_ next');
>+ return;
>+ }
> cancel_
> cancel_
> 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('
>- window.
>+ link = Y.one('
>+ if (link){
Same for these if-statements.
>+ window. location. assign( link.get( 'href') ); #batchnav_ previous' )){ location. assign( link.get( 'href') ) #batchnav_ previous' ); location. assign( link.get( 'href') ); #batchnav_ first') ){ location. assign( link.get( 'href') ) #batchnav_ first') ; location. assign( link.get( 'href') ); #batchnav_ last')) {
> }
> }
> // Shift+Alt+p - Go to previous page in batch
> if (e.shiftKey && e.altKey && e.keyCode == 80) {
>- if (link = Y.one('
>- window.
>+ link = Y.one('
>+ if (link){
>+ window.
> }
> }
> // Shift+Alt+a - Go to first page in batch
> if (e.shiftKey && e.altKey && e.keyCode == 65) {
>- if (link = Y.one('
>- window.
>+ link = Y.one('
>+ if (link){
>+ window.
> }
> }
> // Shift+Alt+l - Go to last page in batch
> if (e.shiftKey && e.altKey && e.keyCode == 76) {
>- if (link = Y.one('
>- ...