[RFC] Cleanup the ACP javascript

[brunoais]

Introduction:
While I was making needed modifications to
adm\style\permissions.js
I found out some code that could actually be cleaner, faster and even become correct.
Using

Code: Select all

for( ... in ...)

for objects that aren't ours is really a nogo.

I also found some stuff that could use some improvements.
We don't need as much inline scripts as the ACP has.

in adm\style\forum_fn.js
we have a bad example of browser sniffing. (is_ie)

adm\style\editor.js
could use some of this too.

Proposal:
Cleanup the file permissions.js with cleaner code and, if possible, faster to execute code.
The objective here is to fix errors and make everything as light as possible without compromising significantly the readability of the code.

E.g.
Remove all for(... in ...) with a proper for loop
Remove unneeded if's
Change the variable's names so that they become more clear. E.g. rb is not a good variable name (even in its context).


Any more places you think the js needs cleanup?

Note: I can do this myself.

[Oleg]

(12:57:45) brunoais: anything to say about this?
(12:57:45) brunoais: viewtopic.php?f=108&t=42549
(12:58:27) nn-: brunoais: please add a space after ], this also goes for your commit messages
(12:59:12) brunoais: k
(12:59:55) nn-: generally speaking "if it's not broken don't fix it"
(13:00:14) nn-: is there a problem that this is going to solve?
(13:00:41) nn-: i kind of like the variable name fixes
(13:01:16) brunoais: there's a decent amount of code that is unreadable without re-reading many times
(13:01:18) nn-: we also are supposedly moving toward using jquery, there should be an investigation if any of our own js can be thrown out and replaced with jquery calls
(13:01:41) brunoais: do it, then
(13:02:02) nn-: i am doing hooks
(13:02:09) nn-: and merging pull requests
(13:02:22) brunoais: I don't want not to have my code added to 3.1 just because it uses the native API and not jQuery and it should have used jQuery
(13:02:52) nn-: do you know anything about jquery?
(13:03:23) brunoais: I know a lot still, I'm not a pro with jQuery
(13:03:48) brunoais: hum... about 1,5 years of experience with jQuery
(13:04:28) nn-: i am ok with viewtopic.php?f=108&t=42549
(13:04:35) brunoais: my experience tells me that jQuery is getting better and faster but is still too slow to some stuff
(13:05:21) nn-: jquery stuff is being driven by igorw/callumacrae, i don't have much to do with it
(13:05:31) brunoais: k
(13:06:01) nn-: the issue basically is the dev team should be working on hooks, not these js changes
(13:06:17) brunoais: ok
(13:06:24) brunoais: I'll skip that to another time
(13:06:36) brunoais: I'll have to finish this new feature first
(13:06:50) brunoais: I still have a nice amount of time to get used to phpbb
(13:07:06) brunoais: only then I should do something as advanced as that looks

[hanakin]

per the conversation about using jquery if their is a more efficent way of writing somethign that does not us jquery way it against a few things.

1. Is the code completely externalized(does not require the use of inline code)?
2. Is the code more complex or difficult to understand legiably compared to the jQuery?
3. How much more efficient is it?
4. Does it conflict with the use of jQuery or other code?

if you consider these factors when rewriting code then I do not think it is really an issue go ahead I say. Ultimately its up to the Devs but should be fine

[brunoais]

1. Is the code completely externalized(does not require the use of inline code)?

This will require a small amount (even smaller than now) of inline code used only for communication between PHP and js.

2. Is the code more complex or difficult to understand legiably compared to the jQuery?

It has more amount of code (nº of characters) but it isn't harder to understand than when using jQuery.

3. How much more efficient is it?

Depends... Some stuff can get to 20x faster usually it stays with 3-7x faster (note that this number depends on the browser's js engine)

4. Does it conflict with the use of jQuery or other code?

Not at all. My code is meant to be fast, clean and conflict free with everything else. If any of those does not happen, then it really is a mistake of my part that needs fixing.

if you consider these factors when rewriting code then I do not think it is really an issue go ahead I say. Ultimately its up to the Devs but should be fine

I will!

[hanakin]

1. Is the code completely externalized(does not require the use of inline code)?

This will require a small amount (even smaller than now) of inline code used only for communication between PHP and js.

then use jQuery your version fails to meet requirements

the amount of inline code currently being used is actually enormous so your statement is unjust by that alone, there is absolutely no reason to use code in the templates themselves.

if you need to comunicate with php use an event trigger to run an ajax call.

personally I am of the mind set that the speed enhancements of not using jQuery far out way the simplicity and standardization of the code from a theme design/development standpoint the increase amount of time it takes to load based on a single instance of using jQuery over not can be made up for through several other methods of server load reduction and what not seeing as how the current theme only achieves a 73% rating though YSlow there is ample room elsewhere we can save resources/bandwidth

I know igor is making a huge push for everything to eventually be jQuery with exception perhaps for the plugin/modular stuff like the editor. The debate on this is really over that is why jQuery was merged in the first place

[hanakin]

perfect example I just through this together,

currently prosilver creates an href for each code box generated in a post that uses an onclick call to execute the following code

Code: Select all

function selectCode(a)
{
	// Get ID of code block
	var e = a.parentNode.parentNode.getElementsByTagName('CODE')[0];

	// Not IE and IE9+
	if (window.getSelection)
	{
		var s = window.getSelection();
		// Safari
		if (s.setBaseAndExtent)
		{
			s.setBaseAndExtent(e, 0, e, e.innerText.length - 1);
		}
		// Firefox and Opera
		else
		{
			// workaround for bug # 42885
			if (window.opera && e.innerHTML.substring(e.innerHTML.length - 4) == '<BR>')
			{
				e.innerHTML = e.innerHTML + '&nbsp;';
			}

			var r = document.createRange();
			r.selectNodeContents(e);
			s.removeAllRanges();
			s.addRange(r);
		}
	}
	// Some older browsers
	else if (document.getSelection)
	{
		var s = document.getSelection();
		var r = document.createRange();
		r.selectNodeContents(e);
		s.removeAllRanges();
		s.addRange(r);
	}
	// IE
	else if (document.selection)
	{
		var r = document.body.createTextRange();
		r.moveToElementText(e);
		r.select();
	}
}

this is bad for several reasons fist of all it is a horribly inefficient function secondly it uses inline scripting to attach it self to the DOM.

instead we can use something like this via jQuery

Code: Select all

(function() {
	var selector = $('dl.codebox');
	$('<a href="#">SELECT ALL</a>').appendTo(selector.children('dt')).on('click', function() {
		var code = $(this).parents(selector).find('code')[0];
		if (document.body.createTextRange) { // ms
			var range = document.body.createTextRange();
			range.moveToElementText(code);
			range.select();
		} else if (window.getSelection) { // moz, opera, webkit
			var selection = window.getSelection(),            
			range = document.createRange();
			range.selectNodeContents(code);
			selection.removeAllRanges();
			selection.addRange(range);
		}
	})
})();

have not done extensive testing with theme and on actual board but should work

[A_Jelly_Doughnut]

Your proposed replacement function would need to be run through the template parser to take care of localisation (for your SELECT ALL). Does that affect your opinion? Or was that just a quickly thrown together example and you would really attach the event to a class? (I'm guessing the second one)

[hanakin]

yes the second, we really do not need to display the select all if JS is not present. i made this via a simplified mosck up of how phpbb handles it. We can simply render it as a hidden element and then just toggle it on but still makes my point