r69444 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r69443‎ | r69444 | r69445 >
Date:21:55, 16 July 2010
Author:platonides
Status:ok (Comments)
Tags:
Comment:
Make addPortletLink() work even if the ul is not there.
Insert the new ul into the last div if there's one (typically a <div class="menu">), directly in the portlet otherwise.
Modified paths:
  • /trunk/phase3/skins/common/wikibits.js (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/common/wikibits.js
——@@ -323,7 +323,25 @@
324324 if ( !root ) {
325325 return null;
326326 }
327 - var node = root.getElementsByTagName( 'ul' )[0];
 327+ var uls = root.getElementsByTagName( 'ul' );
 328+ var node;
 329+ if (uls.length > 0) {
 330+ node = uls[0];
 331+ } else {
 332+ node = document.createElement('ul');
 333+ var lc = null;
 334+ for (var i in root.childNodes) { /* get root.lastElementChild */
 335+ if (root.childNodes[i].nodeType == 1) {
 336+ lc = root.childNodes[i];
 337+ }
 338+ }
 339+ if (lc && lc.nodeName.match(/div/i)) {
 340+ /* Insert into the menu divs */
 341+ lc.appendChild(node);
 342+ } else {
 343+ root.appendChild(node);
 344+ }
 345+ }
328346 if ( !node ) {
329347 return null;
330348 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r69633Follow up r69444. Address CR comment by Simetrical.platonides17:35, 20 July 2010

Comments

#Comment by Simetrical (talk | contribs)   23:53, 19 July 2010 
+		for (var i in root.childNodes) { /* get root.lastElementChild */
+			if (root.childNodes[i].nodeType == 1) {
+				lc = root.childNodes[i];

Isn't this type of for loop discouraged in JavaScript, since it will return any properties the array has as well as its elements? Also, if i is an element of root.childNodes, shouldn't the next lines use "i" instead of "root.childNodes[i]"? See e.g. <https://developer.mozilla.org/En/DOM/NodeList>: "Don't be tempted to use for...in or for each...in to enumerate the items in the list, since that will also enumerate the length and item properties of the NodeList and cause errors if your script assumes it only has to deal with element objects."

Also, I don't know what "lc" stands for. Could you use a longer and more descriptive variable name?

And you're not following style guidelines – not enough spaces inside parentheses.

#Comment by Platonides (talk | contribs)   17:36, 20 July 2010

> Also, I don't know what "lc" stands for. Could you use a longer and more descriptive variable name?

last child

> shouldn't the next lines use "i" instead of "root.childNodes[i]"?

No, unlike other languages, i is the index, not the item itself. But you are right, it is also returning things like item and length.

Fixed with the other issues in r69633.

#Comment by Simetrical (talk | contribs)   01:00, 21 July 2010

Okay, thanks.

#Comment by Catrope (talk | contribs)   17:44, 20 July 2010

Simetrical is right, you should not be using a for .. in loop to iterate over an array, because that'll also include things people add to Array.prototype .

#Comment by Catrope (talk | contribs)   17:44, 20 July 2010

Never mind, was fixed already.

Status & tagging log