| Project: | Localization client |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
Issue Summary
I found the structure of l10n_client.js rather weird, especially in comparison to other projects
afaict, it puts the main function in Drupal.behaviors (as it should), but it puts the child functions and settings in Drupal.l10nClient, and it does this by pulling 1 major jquery extend on the Drupal object... it seems cheaper to just set it straight on the object instead of extending it, this namespace shouldn't be used by anything else anyway
so while I was at it, I reformed it to the usual Drupal.behaviors.l10nClient = function(context) { ... }; and Drupal.behaviors.l10nClient.toggle = function(state) { ... } which makes for a much easier to read file
I also cached $('#l10n-client') as $client in most places, so the selector doesn't need to be re-evaluated every time, things like $('#l10n-client .labels .toggle') was reformed to $client.find('.labels .toggle') which should also boost performance a tiny bit
you'll also notice I created a reference to Drupal.behaviors.l10nClient as self so Drupal.behaviors.l10nClient.toggle(state) can be called as self.toggle(state), making it a lot less verbose
I also noticed that it was creating userSelection as a global variable, which is just bound to conflict sometime
there's still a lot of room for improvement, some selectors like #l10n-client-form are still being called a ton of times and the settings should probably act as defaults, merging in with Drupal.settings.l10nClient, so if any module has set them explicitly, it would override the defaults
but I didn't want to make the patch too huge, which it probably already is... oh well
attached patch should apply to the 6.x dev
| Attachment | Size |
|---|---|
| l10n_client.js_.cleanup.patch | 11.15 KB |
Comments
#1
Generally looks good, although you seem to have not used "self" consistently. I just cut a new D6 release for the module since it had important fixes and improvements in the dev version that people were not actively using. Now I'd like to sync up the D7 version, since it has a very significant lag behind the D6 version in terms of bug fixes ported, and then we can look at making these changes in D6 and D7 too I'd say. Agreed that the patch is big enough (and we need to make the changes in D6 and D7 in parallel).
#2
so postponed until we got the D6 & D7 versions synced?
#3
Well, I did lots of work on this today, and now it is definitely not far :) Only #329858: Uninstall code is missing and #361147: Support for different text groups is outstanding. The first is minimal, the second is ported but needs some work (its close).
#4
awesome, do you think I should keep working on this in parallel or perhaps split it up into several stages so it isn't one big bulky patch?
#5
Several stages would be very helpful, also easier to apply/port to D6 and D7 simultaneously.
#6
roger!
I'd say getting rid of that global would be first priority, it's been poking my eyes out :P
#7
@seutje: looking forward to the improvements :)
#8
crap, totally forgot about this :x
I'll grab a clone right now for on the train so I don't forget!
#9
global be gone!
#10
cached
$('#l10n-client')selector and changed 2 instanced of$('#l10n-client #l10n-client-search-filter-clear')to$('#l10n-client-search-filter-clear'), being a (by definition) unique ID, I think we can assume it will only occur within the client#11
this is probably a good start, might continue this on my commute back home
#12
forgot to use the cached selector in 2 instances in the patch in #10
1st attached patch fixes that
2nd attached patch also caches stringlist selector
both patches sort of assume the previous ones went in
bastard coworkers keep giving me a lift home, cutting down commute time :P
#13
also cached the form selector. Again, this assumes the previous ones went in
as u've prolly noticed by now, having the thing live in multiple locations within the Drupal object makes it rather annoying to reuse variables.
next step will prolly be to put everything in
Drupal.behaviors.l10nClient, have the sub-functions be methods and the variables be properties of that object, that way we could reference it internally likeself.formandself.clientwhich should make it a lot easier to readthis would also theoretically allow other modules/scripts to manipulate this by referencing
Drupal.behaviors.l10nClient.formor something...Considering the location within the object would change and most of the indentation would change, I'm afraid this next step will be a rather large chunk no matter how I go about it
#14
Ok, great, thanks. Let's stop here for a bit, so we can apply these to Drupal 6 and 7 and then continue :)
#15
hokai, u can prolly just merge them into the 7.x branch, right?
or should I make ports for each of them?
#16
hmz, looks like line 71 of the 6.x branch (
$('#l10n-client #search-filter-clear').focus();) and line 73 of the 7.x branch ($('#l10n-client #l10n-client-search-filter-clear').focus();) will prolly break a seamless merge for the patch in #10and I just realised I've been patching the 7.x branch... >_<
if u want me to reroll the patch in #10 for the 6.x branch, lemme know
#17
Ok, well. Looks like your patches did not apply to the Drupal 7 version either. Here is a roll of your patches (I think) to the Drupal 7 version. However, it breaks at least the initial bar behavior, the bar looks odd (with all the header text) and it does not open/close on click. Don't have lots of time to debug now, but it definitely worked before the patch. Tried to roll all your patches into one, so this is all of the above (that is since #9) rolled into one applying to the Drupal 7 branch. Can we (a) make this work (b) commit (c) get it ported to Drupal 6 (d) commit, (e) then work on further improvements :) Thank you!
#18
oh my, sorry about that, not sure what went wrong
is it maybe because I based it on the git.drupalcode.org repo?
will look into this as soon as I find a minute :x
#19
$.extend(Drupal, {l10nClient: new (function() {
+ // Cache selector.
+ var $client = $('#l10n-client');
Is that execute before DOM ready ?? so $client catch nothing
#21
@sutje: could not find time to get back to this? It would be great if you could! Thanks!
#22
k, another try
getting rid of the extend first this time, should apply to the 7.x branch of the git.drupalcode.org
#23
got rid of global
complete + incremental patch
#24
shallow cache of the client object
complete + incremental
#25
more optimization
#26
nice, thanks!
should prolly also cache those repeating
$(this)calls in the submit handler#27
This don't fix the d7 version,
It crashes the Seven Theme Javascripts.
I also get this error
$.cookie is not a function
switch($.cookie('Drupal_l10n_client')) {
#28
@unegro: for that, see #522646: jquery.cookie.js incompatible with Apache mod_security by default.
#29
also cached
$(this),
$l10nClientForm
and use this keyword
and cached $('#l10n-client') in Drupal.l10nClient.ClientObj()
#30
Is this patch for drupal 7?
I don't find the dirs a/l10n_client.js b/l10n_client.js
I dont understood the command in line 5 of your patch...
diff --git a/l10n_client.js b/l10n_client.js
index df08e05..377d4f7 100644
--- a/l10n_client.js
+++ b/l10n_client.js
thanks
#31
@ugegro,
are you review my patch ? If not, Google "how to apply a patch"
It's patch won't fix any error in this module but make the code more clean & better. If you face problem within module, crate a new issue.
#32
@Droplet: thanks man, looking a lot cleaner!
@unegro: see docs on git apply -> http://www.kernel.org/pub/software/scm/git/docs/git-apply.html
#33
I'm afraid I just broke the patch with #1184960: Add context support to edit and save strings., which was important to commit soon sorry :) Can you please update the patch with changes from there included? Thanks!
#34
OK.
- added #1184960 changed
- use .class to styling element
I left one because it have buggy. I will fix on this new issue: #1195928: rendering bug on IE > 6
#35
get rid of Tabs
#36
@Gabor #28
I don't think this has anything to do with apache security since you can use $.cookie just before you add the anonymous function to Drupal.behaviors.l10nClient (87) but not within this function.
The least you can say is that cookie.js is indeed loaded but that scope to jquery with cookie functionality is lost inside the function.
try
$.cookie('testMe','bake cookie');
alert($.cookie('testMe'));
and you can see that it works at line 86 (but still breakes at line 101) and that the snippet breaks if you put it inside the function.
"cookie is not a function" can break other scripts as reported by others.
#37
was there a new version of l10n_client 6.x-2.0 today ? Seems fixed.
#38
quit hijacking this issue please
#39
+++ b/l10n_client.jsundefined@@ -2,98 +2,100 @@
+ $('body').css('border-bottom', '22em solid #fff');
Please, please do this a different way.
#40
that line was untouched, only reason it gets -'d and +'d is cause the indentation is different
although I agree that using an inline style isn't ideal, I deliberately didn't make any changes like that
the changes I did make shouldn't change anything in behavior, but are mainly about performance and readable code
adding changes like that would make this issue infinitely more complex