r86047 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r86046‎ | r86047 | r86048 >
Date:13:18, 14 April 2011
Author:diebuche
Status:reverted (Comments)
Tags:
Comment:
History: Adding JS that converts buttons to links for better UX. Patch based on one by Matthew Flaschen. Fixes Bug 16165
Modified paths:
  • /trunk/phase3/resources/mediawiki.action/mediawiki.action.history.js (modified) (history)
  • /trunk/phase3/skins/common/history.js (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/common/history.js
——@@ -1,6 +1,6 @@
22 window.historyRadios = function( parent ) {
3 - var inputs = parent.getElementsByTagName( 'input' );
4 - var radios = [],
 3+ var inputs = parent.getElementsByTagName( 'input' );
 4+ var radios = [],
55 i = 0;
66 for ( i = 0; i < inputs.length; i++ ) {
77 if ( inputs[i].name == 'diff' || inputs[i].name == 'oldid' ) {
——@@ -76,4 +76,84 @@
7777 }
7878 }
7979 return true;
80 -};
\ No newline at end of file
 80+};
 81+
 82+//update the compare link as you select radio buttons
 83+window.updateCompare = function () {
 84+ var hf = compareLink.$form.get(0);
 85+ var oldInd = -1;
 86+ var i = 0;
 87+ while(oldInd == -1 & i<hf.oldid.length) {
 88+ if(hf.oldid[i].checked){
 89+ oldInd=i;
 90+ }
 91+ i++;
 92+ }
 93+ var diffInd=-1;
 94+ var j=0;
 95+ while(diffInd==-1 & j<hf.diff.length) {
 96+ if(hf.diff[j].checked){
 97+ diffInd=j;
 98+ }
 99+ j++;
 100+ }
 101+ var wikiLinkURL = wgServer + wgScript + "?title=" + encodeURIComponent(hf.title.value)
 102+ + "&diff=" + hf.diff[diffInd].value + "&oldid=" + hf.oldid[oldInd].value;
 103+ compareLink.wikiTop.attr("href", wikiLinkURL);
 104+ compareLink.wikiEnd.attr("href", wikiLinkURL);
 105+
 106+ if(compareLink.htmlDiffs)
 107+ {
 108+ var htmlLinkURL = wgServer + wgScript + "?title=" + encodeURIComponent(hf.title.value)
 109+ + "&htmldiff=" + compareLink.htmlDiffButtonTxt
 110+ + "&diff=" + hf.diff[diffInd].value + "&oldid=" + hf.oldid[oldInd].value;
 111+ compareLink.htmlTop.attr("href", htmlLinkURL);
 112+ compareLink.htmlEnd.attr("href", htmlLinkURL);
 113+ }
 114+};
 115+
 116+//change the button to a link where possible
 117+window.fixCompare = function () {
 118+ window.compareLink = {};
 119+ var doneHtml = false;
 120+ var doneWiki = false;
 121+ compareLink.htmlDiffs = false;
 122+
 123+ compareLink.$form = $("#mw-history-compare");
 124+ var hf = compareLink.$form.get(0);
 125+
 126+ var $buttons = $('input.historysubmit');
 127+
 128+ if (!compareLink.$form.length || !$buttons.length) return;
 129+
 130+ $buttons.each(function() {
 131+ if(this.name == "htmldiff") {
 132+ if (doneHtml) return true;
 133+ doneHtml = true;
 134+ var url = wgServer + wgScript + "?title=" + encodeURIComponent(hf.title.value)
 135+ + "&htmldiff=" + this.value + "&diff=" + hf.diff[0].value + "&oldid=" + hf.oldid[1].value;
 136+ compareLink.htmlDiffs = true;
 137+ } else {
 138+ if (doneWiki) return true;
 139+ doneWiki = true;
 140+ var url = wgServer + wgScript + "?title=" + encodeURIComponent(hf.title.value)
 141+ + "&diff=" + hf.diff[0].value + "&oldid=" + hf.oldid[1].value;
 142+ }
 143+ var $linkTop = $("<a href='" + url + "' class='historycomparelink ui-button'>" + this.value + "</a>").button();
 144+ compareLink.$form.before($linkTop);
 145+ $linkEnd = $linkTop.clone();
 146+ compareLink.$form.append($linkEnd);
 147+
 148+ if(this.name == "htmldiff") {
 149+ compareLink.htmlTop = $linkTop;
 150+ compareLink.htmlEnd = $linkEnd;
 151+ } else {
 152+ compareLink.wikiTop = $linkTop;
 153+ compareLink.wikiEnd = $linkEnd;
 154+ }
 155+ });
 156+ $buttons.hide();
 157+
 158+ $("#pagehistory").change(function() {window.updateCompare()});
 159+};
 160+
Index: trunk/phase3/resources/mediawiki.action/mediawiki.action.history.js
——@@ -5,4 +5,7 @@
66 // Replaces histrowinit
77 $( '#pagehistory li input[name="diff"], #pagehistory li input[name="oldid"]' ).click( diffcheck );
88 diffcheck();
 9+ mediaWiki.loader.using('jquery.ui.button', function() {
 10+ window.fixCompare();
 11+ });
912 });
\ No newline at end of file

Follow-up revisions

RevisionCommit summaryAuthorDate
r86050Followup to r86047: Declare dependency to JUI button more cleanlydiebuche13:52, 14 April 2011
r86056Followup to r86047: Rewrite the radio-button updater & move everything to act...diebuche16:06, 14 April 2011
r86188Followup to r86047: Fix empty button on pages with only one revdiebuche08:44, 16 April 2011
r91547Revert r86047 for now per CR.diebuche12:29, 6 July 2011

Comments

#Comment by He7d3r (talk | contribs)   13:36, 14 April 2011

FYI: See also the version at Snippets/Compare link

#Comment by DieBuche (talk | contribs)   14:02, 14 April 2011

Nice! It's much more concise. It doesn't support the HTML diff button though

#Comment by Krinkle (talk | contribs)   15:10, 14 April 2011

Please don't create new global legacy variables, or anything new in legacy files for that matter.

Neither fixCompare, compareLink or updateCompare is globally (window.) needed. They are not referenced outside of their current scope (ie. another file)

Instead define them locally (var) in mediawiki.action.history.js 

var updateCompare = function(){
};
var compareLink = {};
var fixCompare = funciton(){
  compareLink.....
};
..
fixCompare();

or, if needed outside this scope: 

var compareLink = {};
mw.history = {};
mw.history.updateCompare = function(){
};
mw.history.fixCompare = funciton(){
  compareLink.....
};
..
mw.history.fixCompare();

The only globals that should be created/used in core are mw (mediaWiki) and jQuery. Everything else is either part of those objects or local, or aliased.

mediawiki.action.history.js already has an alias from jQuery to $, so $ can be used safely here.

Also, there's no need for an additional anonymous function for the call, and the fullname global 'mediaWiki' (alias mw is globally available). 

+	mediaWiki.loader.using('jquery.ui.button', function() {
+		window.fixCompare();
+	});

Functions are objects and are always passed by reference, the following is more effecient 

+	mw.loader.using('jquery.ui.button', window.fixCompare );

This is creating an additional http request for all history pages though, should be added as a dependancy on the server side instead.

#Comment by DieBuche (talk | contribs)   15:13, 14 April 2011

Yep, I realized his as well. I'm reqriting it and moving it to action.history.js

#Comment by DieBuche (talk | contribs)   16:08, 14 April 2011

All done in r86056

#Comment by Krinkle (talk | contribs)   15:11, 14 April 2011

I see the latter has been addressed in r86050 already.

#Comment by Krinkle (talk | contribs)   15:15, 14 April 2011

Something else real quick, be careful not to reference deprecated functionality.

  • wgScript is a deprecated global as of 1.17, use mw.config instead (RL/JD)
  • Take the time to read the Manual:Coding conventions for core php and javascript. There's some minor whitespace issues.
#Comment by 😂 (talk | contribs)   15:30, 14 April 2011

HTMLDiff was killed, you can remove anything relating to that.

#Comment by Duplicatebug (talk | contribs)   08:23, 16 April 2011

Please have a look at pages with one version. &diff=undefined&oldid=undefined looks bad.

#Comment by Bawolff (talk | contribs)   22:43, 29 April 2011

This makes things look weird when one has revdel related rights since we get native vs weird css buttons right beside each other that look slightly different. OTOH, the history page in general looked kind of ugly even before this, when you have revdel rights.

Personally I'm unclear on how this improves usability. Whats the difference between a button and a link dressed up as a button using css?

#Comment by He7d3r (talk | contribs)   16:39, 30 April 2011

For me, the main advantages of a link are:

  • With a link I can open the diffs in a new tab; and
  • I can also copy the link without having to open it before.

I'm not able to do any of this with a button (is there a way?).

#Comment by Krinkle (talk | contribs)   18:19, 5 July 2011

The first one can be done with a button fine (in recent browsers). When clicking the button (ie. "submitting the ‎<form>)") you can hold the middle button of your mouse or cmd-key (or whatever method you use to open a normal link in a new tab) while clicking the button and it should submit it to that tab.

I agree a link has advantages but the problem with this is jquery.ui.button implementation is that:

  • It looks a like a button, people expect button behaviour not link behaviour (except for power users)
  • It's a button unlike any other button in the MediaWiki interface
  • It will either flash for a bit or it will load all of jQuery UI in the ‎<head>.

I suggest reverting this because it may confuse new users (it doens't behave like a button yet looks like one, one that is not in style with their OS nor with the MediaWiki style guide or anything in the MediaWik site).

Instead wait for the new style HTML-forms are ready in trunk which will give all buttons the same look and will make it possible to re-do this in a way that will give best of both (ie. it's still a button but will enable doing links and tabs easily).

#Comment by He7d3r (talk | contribs)   20:20, 5 July 2011

For the record: clicking with the middle button of mouse in a button using any of these browsers:

  • Google Chrome 12.0.742.112
  • Opera/9.80 (Windows NT 5.1; U; pt-BR) Presto/2.9.168 Version/11.50
  • Mozilla/5.0 (Windows; U; Windows NT 5.1; pt-BR; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
  • IE 8.06001.18702

doesn't submit to a new tab in. Insted, it starts that feature which allows the user to scroll the page up/down while moving the mouse.

Using the same button of the mouse to click in a link open it on a new tab on all browsers mentioned above.

#Comment by DieBuche (talk | contribs)   12:34, 6 July 2011

Reverted until we have uniform buttons sitewide. In atleast Chrome & Safari it's possible to command (on a mac, no idea which key on win) click a button to open in a new tab.

Status & tagging log