r114064 MediaWiki - Code Review archive

Seeing the way i18n is handled here, makes me sad.

...really? I've read the docs, and I thought I was following them. If what I'm doing here is wrong or not helpful, I'd like to know why and how I can fix it.

+ 'articlefeedbackv5-disable-flyover-help-goto' => 'To remove $1, go to',
+ 'articlefeedbackv5-disable-flyover-help-emphasis-text' => 'Article Feedback',
+ 'articlefeedbackv5-disable-flyover-help-location' => 'My preferences > Appearance',
+ 'articlefeedbackv5-disable-flyover-help-direction' => 'and check',
+ 'articlefeedbackv5-disable-flyover-prefbutton' => 'Go to my preferences',

This is one big pile of Lego. You should have a master message that looks like 'To remove $1, got to $2 and check $3' rather than concatenating things together with the hardcoded assumption that they'll always be in that order in every single one of the 250+ languages we support.

+.tipsy-arrow {
/* @embed */
background-image: url(images/tipsy-flyover.png);
width: 23px;
height: 11px;
+}
+.tipsy-nw .tipsy-arrow {
+ margin-top: -6px;
+}
+.tipsy-se .tipsy-arrow {
margin-top: -11px;
}

The CSS rules for Tipsy here should be namespaced (made more specific) so they only apply to Tipsy tooltips created by AFTv5, not to any other Tipsy tooltips that might appear on the page.

+ var re = new RegExp("\\$" + i);
+ full = full.replace( re, mw.html.element(

The replace function takes a string too, so you can do full.replace( '$' + i, ... ) and that's equivalent to this code. That will only replace the first occurrence though; if you care about replacing multiple occurrences of the same parameter, use the g modifier in your regex: re = new RegExp("\\$" + i, 'g');.

However, this still suffers from a potential bug where $1 expands to something containing the literal string "$2", which is then expanded again, and it's also buggy if $10 is used (because that'll get expanded as $1 followed by a literal 0). If you're paranoid enough to want to fix that (or just like cleaner code), you can use a callback with replace():

full.replace( /\$(\d+)/g, function( str, number ) {
    var i = parseInt( number, 10 );
    return whatTheReplacementShouldBeFor( i );
} );

+ hasTipsy = false;

AFAICT this variable is leaked to the global scope.

Annoyingly enough, your suggestion was what I did first, before I noticed that the mockups called for line breaks. I would LOVE to put it back the way you suggest, but Fabrice needs to sign off on not having the line breaks he wanted.

I'll fix the other stuff soon.

Fixed in followups; marking back to new.