"Catrope" changed the status of MediaWiki.r114064 to "fixme" and commented it.
URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/114064#c32393
Old Status: new
New Status: fixme
Commit summary for MediaWiki.r114064:
Brought trigger links into the plugin itself; styled flyover panel:
- modules/jquery.articleFeedbackv5/jquery.articleFeedbackv5.js:
* Brought in trigger links, standardized them, and added the
disable
flyover.
- New property selectedLinks
- New template disableFlyover
- Added object triggerLinks (like buckets and ctas)
- Added selecting and insertion of trigger links to init()
- New methods selectTriggerLinks(), addTriggerLinks(),
buildDisableFlyover(), and clickTriggerLink()
- Updated buildLink() to handle a flexible number of
replacements, and
to handle tags other than links
- modules/jquery.articleFeedbackv5/jquery.articleFeedbackv5.css:
- Updated flyover styles to match latest mockup and to work
with both
link E and link A
- Added trigger link styles from ext.articleFeedbackv5.css
- modules/ext.articleFeedbackv5/ext.articleFeedbackv5.css:
- Removed trigger link styles
- modules/ext.articleFeedbackv5/ext.articleFeedbackv5.js:
- Removed building of trigger links
- Plugin is now attached after it's been added to the page, so
that
trigger links can be inserted relative to it
- ArticleFeedbackv5.i18n.php:
- Changed messages and wording to match latest mockup
- Put in real doc lines
- ArticleFeedbackv5.hooks.php:
- Updated to match new messages
Catrope's comment:
<pre>
+ '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',
</pre>
This is one big pile of Lego. You should have a master message that looks like
<code>'To remove $1, got to $2 and check $3'</code> 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.
<pre>
+.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;
}
</pre>
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.
<pre>
+ var re = new RegExp("\\$" + i);
+ full = full.replace( re, mw.html.element(
</pre>
The replace function takes a string too, so you can do <code>full.replace( '$'
+ i, ... )</code> 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: <code>re = new
RegExp("\\$" + i, 'g');</code>.
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():
<pre>
full.replace( /\$(\d+)/g, function( str, number ) {
var i = parseInt( number, 10 );
return whatTheReplacementShouldBeFor( i );
} );
</pre>
<pre>
+ hasTipsy = false;
</pre>
AFAICT this variable is leaked to the global scope.
_______________________________________________
MediaWiki-CodeReview mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
