From 40eca4e3f30ada0c802b2a1ff18d114877c302cd Mon Sep 17 00:00:00 2001 From: jdlrobson Date: Thu, 16 Aug 2018 12:00:38 -0700 Subject: [PATCH] Hygiene: IssuesOverlay moved from MobileFrontend to Minerva The CleanupOverlay is moved to Minerva and renamed the IssuesOverlay to be consistent with current terminology The new IssuesOverlay is defined inside the module skins.minerva.scripts to which it now belongs. Additional changes: * various file renames * overlay-cleanup renames overlay-issues * cleanuptemplates renamed issues.js * Add a test stub file to avoid the need to load templates inside the test environment After this change, I75f47622d94e504688e04dfb2892540473817053 should be merged to avoid confusion. Change-Id: I08945a324a6b878abe56efed1e988466085b3018 --- includes/MinervaHooks.php | 16 ++-- resources/skins.minerva.editor/init.js | 2 +- .../PageIssuesOverlay.js | 94 +++++++++++++++++++ .../PageIssuesOverlay.less | 54 +++++++++++ .../PageIssuesOverlayContent.hogan | 10 ++ resources/skins.minerva.scripts/init.js | 2 +- .../{cleanuptemplates.js => pageIssues.js} | 36 +++---- ...pageIssueLogger.js => pageIssuesLogger.js} | 2 +- ...pageIssueParser.js => pageIssuesParser.js} | 2 +- skin.json | 14 ++- tests/qunit/skins.minerva.scripts/stubs.js | 1 + ...cleanuptemplates.js => test_pageIssues.js} | 2 +- ...ssueParser.js => test_pageIssuesParser.js} | 12 +-- 13 files changed, 206 insertions(+), 41 deletions(-) create mode 100644 resources/skins.minerva.scripts/PageIssuesOverlay.js create mode 100644 resources/skins.minerva.scripts/PageIssuesOverlay.less create mode 100644 resources/skins.minerva.scripts/PageIssuesOverlayContent.hogan rename resources/skins.minerva.scripts/{cleanuptemplates.js => pageIssues.js} (92%) rename resources/skins.minerva.scripts/{pageIssueLogger.js => pageIssuesLogger.js} (98%) rename resources/skins.minerva.scripts/{pageIssueParser.js => pageIssuesParser.js} (98%) create mode 100644 tests/qunit/skins.minerva.scripts/stubs.js rename tests/qunit/skins.minerva.scripts/{test_cleanuptemplates.js => test_pageIssues.js} (94%) rename tests/qunit/skins.minerva.scripts/{test_pageIssueParser.js => test_pageIssuesParser.js} (91%) diff --git a/includes/MinervaHooks.php b/includes/MinervaHooks.php index 2df6164..6583c0c 100644 --- a/includes/MinervaHooks.php +++ b/includes/MinervaHooks.php @@ -75,24 +75,26 @@ class MinervaHooks { 'mobile.startup', 'skins.minerva.notifications.badge', 'mediawiki.user', - 'mediawiki.experiments', - 'mobile.issues' + 'mediawiki.experiments' ], 'localBasePath' => dirname( __DIR__ ), 'remoteSkinPath' => 'MinervaNeue', 'targets' => [ 'mobile', 'desktop' ], 'scripts' => [ // additional scaffolding (minus initialisation scripts) - 'resources/skins.minerva.scripts/pageIssueLogger.js', - 'resources/skins.minerva.scripts/pageIssueParser.js', + 'tests/qunit/skins.minerva.scripts/stubs.js', + + 'resources/skins.minerva.scripts/pageIssuesLogger.js', + 'resources/skins.minerva.scripts/pageIssuesParser.js', 'resources/skins.minerva.scripts/DownloadIcon.js', 'resources/skins.minerva.scripts/AB.js', - 'resources/skins.minerva.scripts/cleanuptemplates.js', + 'resources/skins.minerva.scripts/PageIssuesOverlay.js', + 'resources/skins.minerva.scripts/pageIssues.js', // test files 'tests/qunit/skins.minerva.scripts/test_DownloadIcon.js', - 'tests/qunit/skins.minerva.scripts/test_pageIssueParser.js', + 'tests/qunit/skins.minerva.scripts/test_pageIssuesParser.js', 'tests/qunit/skins.minerva.scripts/test_AB.js', - 'tests/qunit/skins.minerva.scripts/test_cleanuptemplates.js', + 'tests/qunit/skins.minerva.scripts/test_pageIssues.js', 'tests/qunit/skins.minerva.notifications.badge/test_NotificationBadge.js' ], ]; diff --git a/resources/skins.minerva.editor/init.js b/resources/skins.minerva.editor/init.js index efc27ee..59f56fb 100644 --- a/resources/skins.minerva.editor/init.js +++ b/resources/skins.minerva.editor/init.js @@ -6,7 +6,7 @@ isEditable = !isReadOnly && mw.config.get( 'wgIsProbablyEditable' ), blockInfo = mw.config.get( 'wgMinervaUserBlockInfo', false ), router = require( 'mediawiki.router' ), - issues = M.require( 'skins.minerva.scripts/cleanuptemplates' ), + issues = M.require( 'skins.minerva.scripts/pageIssues' ), overlayManager = M.require( 'skins.minerva.scripts/overlayManager' ), loader = M.require( 'mobile.startup/rlModuleLoader' ), Icon = M.require( 'mobile.startup/Icon' ), diff --git a/resources/skins.minerva.scripts/PageIssuesOverlay.js b/resources/skins.minerva.scripts/PageIssuesOverlay.js new file mode 100644 index 0000000..4143a15 --- /dev/null +++ b/resources/skins.minerva.scripts/PageIssuesOverlay.js @@ -0,0 +1,94 @@ +( function ( M ) { + var Overlay = M.require( 'mobile.startup/Overlay' ), + util = M.require( 'mobile.startup/util' ); + + /** + * @typedef {Object} PageIssue + * @property {string} icon html associated with Icon component + * @property {string} text html explaining the details of the issue + * @property {string} severity associated with the issue + */ + + /** + * Obtain severity associated with a given $target node + * by looking at associated parent node (defined by template) + * + * @param {jQuery.Object} $target + * @return {string} severity as defined in associated PageIssue + */ + function parseSeverity( $target ) { + return $target.parents( '.issue-notice' ).data( 'severity' ); + } + + /** + * Overlay for displaying page issues + * @class PageIssuesOverlay + * @extends Overlay + * + * @param {Object} options Configuration options + * @param {string} options.headingText + * @param {PageIssue[]} options.issues list of page issues for display + * @fires PageIssuesOverlay#link-edit-click + * @fires PageIssuesOverlay#link-internal-click + */ + function PageIssuesOverlay( options ) { + options.heading = '' + options.headingText + ''; + Overlay.call( this, options ); + } + + OO.mfExtend( PageIssuesOverlay, Overlay, { + /** + * @memberof PageIssuesOverlay + * @instance + */ + className: 'overlay overlay-issues', + /** + * @memberof PageIssuesOverlay + * @instance + */ + events: util.extend( {}, Overlay.prototype.events, { + 'click a:not(.external):not([href*=edit])': 'onInternalClick', + 'click a[href*="edit"]': 'onEditClick' + } ), + /** + * @memberof PageIssuesOverlay + * @instance + */ + templatePartials: util.extend( {}, Overlay.prototype.templatePartials, { + content: mw.template.get( 'skins.minerva.scripts', 'PageIssuesOverlayContent.hogan' ) + } ), + /** + * Event that is triggered when an internal link inside the overlay is clicked + * This event will not be triggered if the link contains the edit keyword, in which + * case onEditClick will be fired + * This is primarily used for instrumenting page issues + * (see https://meta.wikimedia.org/wiki/Schema:PageIssues) + * @param {jQuery.Event} ev + * @memberof PageIssuesOverlay + * @instance + */ + onInternalClick: function ( ev ) { + /** + * @event PageIssuesOverlay#link-internal-click + * @param {string} severity + */ + this.emit( 'link-internal-click', parseSeverity( this.$( ev.target ) ) ); + }, + /** + * Event that is triggered when an edit link inside the overlay is clicked + * This is primarily used for instrumenting page issues + * (see https://meta.wikimedia.org/wiki/Schema:PageIssues) + * @param {jQuery.Event} ev + * @memberof PageIssuesOverlay + * @instance + */ + onEditClick: function ( ev ) { + /** + * @event PageIssuesOverlay#link-edit-click + * @param {string} severity + */ + this.emit( 'link-edit-click', parseSeverity( this.$( ev.target ) ) ); + } + } ); + M.define( 'skins.minerva.scripts/PageIssuesOverlay', PageIssuesOverlay ); +}( mw.mobileFrontend ) ); diff --git a/resources/skins.minerva.scripts/PageIssuesOverlay.less b/resources/skins.minerva.scripts/PageIssuesOverlay.less new file mode 100644 index 0000000..7d3613a --- /dev/null +++ b/resources/skins.minerva.scripts/PageIssuesOverlay.less @@ -0,0 +1,54 @@ +@import '../../minerva.less/minerva.variables'; + +@smallIconSize: 24px; +@largeIconSize: 50px; + +.mw-mf-cleanup { + display: block; + margin: 0; + padding: 0; + font-size: 0.8em; + color: @colorGray7; +} + +// overlay styles +.overlay-issues { + + .cleanup { + > li { + border-bottom: solid 1px @grayLight; + .issue-notice { + padding: @smallIconSize @smallIconSize @smallIconSize 0; + .mw-ui-icon { + float: left; + } + } + + small, + .hide-when-compact { + font-size: 0.8em; + } + + .hide-when-compact { + display: block; + margin: 8px 0; + } + } + } + + .issue-details { + // align the first line to the top of the element + // but line-height should be normal for text that follows (T190469) + > :first-line { + line-height: 1; + } + + // This should match the icon width for the overlay close icon + // which is derived from these two variables: + padding-left: @iconSize + ( 2 * @iconGutterWidth ); + // assume date + small i { + color: @colorGray7; + } + } +} diff --git a/resources/skins.minerva.scripts/PageIssuesOverlayContent.hogan b/resources/skins.minerva.scripts/PageIssuesOverlayContent.hogan new file mode 100644 index 0000000..67f2a01 --- /dev/null +++ b/resources/skins.minerva.scripts/PageIssuesOverlayContent.hogan @@ -0,0 +1,10 @@ + diff --git a/resources/skins.minerva.scripts/init.js b/resources/skins.minerva.scripts/init.js index 0adcd23..4a04751 100644 --- a/resources/skins.minerva.scripts/init.js +++ b/resources/skins.minerva.scripts/init.js @@ -3,7 +3,7 @@ toast = M.require( 'mobile.startup/toast' ), time = M.require( 'mobile.startup/time' ), skin = M.require( 'mobile.init/skin' ), - issues = M.require( 'skins.minerva.scripts/cleanuptemplates' ), + issues = M.require( 'skins.minerva.scripts/pageIssues' ), DownloadIcon = M.require( 'skins.minerva.scripts/DownloadIcon' ), browser = M.require( 'mobile.startup/Browser' ).getSingleton(), loader = M.require( 'mobile.startup/rlModuleLoader' ), diff --git a/resources/skins.minerva.scripts/cleanuptemplates.js b/resources/skins.minerva.scripts/pageIssues.js similarity index 92% rename from resources/skins.minerva.scripts/cleanuptemplates.js rename to resources/skins.minerva.scripts/pageIssues.js index c10254d..c552d89 100644 --- a/resources/skins.minerva.scripts/cleanuptemplates.js +++ b/resources/skins.minerva.scripts/pageIssues.js @@ -11,9 +11,9 @@ CURRENT_NS = config.get( 'wgNamespaceNumber' ), allPageIssuesSeverity, Icon = M.require( 'mobile.startup/Icon' ), - pageIssueLogger = M.require( 'skins.minerva.scripts/pageIssueLogger' ), - pageIssueParser = M.require( 'skins.minerva.scripts/pageIssueParser' ), - CleanupOverlay = M.require( 'mobile.issues/CleanupOverlay' ), + pageIssuesLogger = M.require( 'skins.minerva.scripts/pageIssuesLogger' ), + pageIssuesParser = M.require( 'skins.minerva.scripts/pageIssuesParser' ), + PageIssuesOverlay = M.require( 'skins.minerva.scripts/PageIssuesOverlay' ), // setup ab test abTest = new AB( { testName: 'WME.PageIssuesAB', @@ -64,7 +64,7 @@ } } ); - pageIssue = pageIssueParser.parse( $box.get( 0 ) ); + pageIssue = pageIssuesParser.parse( $box.get( 0 ) ); return { severity: pageIssue.severity, @@ -138,12 +138,12 @@ allIssues[section] = issues; if ( $metadata.length && inline ) { - severity = pageIssueParser.maxSeverity( + severity = pageIssuesParser.maxSeverity( issues.map( function ( issue ) { return issue.severity; } ) ); new Icon( { glyphPrefix: 'minerva', - name: pageIssueParser.iconName( $metadata.get( 0 ), severity ) + name: pageIssuesParser.iconName( $metadata.get( 0 ), severity ) } ).prependTo( $metadata.find( '.mbox-text' ) ); $learnMore = $( '' ) .addClass( 'ambox-learn-more' ) @@ -156,8 +156,8 @@ $learnMore.appendTo( $metadata.find( '.mbox-text' ) ); } $metadata.click( function () { - var pageIssue = pageIssueParser.parse( this ); - pageIssueLogger.log( { + var pageIssue = pageIssuesParser.parse( this ); + pageIssuesLogger.log( { action: 'issueClicked', issuesSeverity: [ pageIssue.severity ] } ); @@ -169,7 +169,7 @@ // In group B, we link to all issues no matter where the banner is. $link.attr( 'href', '#/issues/' + KEYWORD_ALL_SECTIONS ); $link.click( function () { - pageIssueLogger.log( { + pageIssuesLogger.log( { action: 'issueClicked', // empty array is passed for 'old' treatment. issuesSeverity: [] @@ -267,9 +267,9 @@ if ( isLoggingRequired( getIssues( KEYWORD_ALL_SECTIONS ) ) ) { // Enable logging. - pageIssueLogger.subscribe( + pageIssuesLogger.subscribe( newTreatmentEnabled, - pageIssueLogger.newPageIssueSchemaData( + pageIssuesLogger.newPageIssueSchemaData( newTreatmentEnabled, CURRENT_NS, getIssues( KEYWORD_ALL_SECTIONS ).map( formatPageIssuesSeverity ) @@ -279,7 +279,7 @@ // Setup the overlay route. overlayManager.add( new RegExp( '^/issues/(\\d+|' + KEYWORD_ALL_SECTIONS + ')$' ), function ( section ) { - var overlay = new CleanupOverlay( { + var overlay = new PageIssuesOverlay( { issues: getIssues( section ), // Note only the main namespace is expected to make use of section issues, so the heading will always be // minerva-meta-data-issues-section-header regardless of namespace @@ -288,19 +288,19 @@ } ); // Tracking overlay close event. overlay.on( 'Overlay-exit', function () { - pageIssueLogger.log( { + pageIssuesLogger.log( { action: 'modalClose', issuesSeverity: getIssues( section ).map( formatPageIssuesSeverity ) } ); } ); overlay.on( 'link-edit-click', function ( severity ) { - pageIssueLogger.log( { + pageIssuesLogger.log( { action: 'modalEditClicked', issuesSeverity: [ severity ] } ); } ); overlay.on( 'link-internal-click', function ( severity ) { - pageIssueLogger.log( { + pageIssuesLogger.log( { action: 'modalInternalClicked', issuesSeverity: [ severity ] } ); @@ -309,18 +309,18 @@ } ); // Tracking pageLoaded event (technically, "issues" loaded). - pageIssueLogger.log( { + pageIssuesLogger.log( { action: 'pageLoaded', issuesSeverity: allPageIssuesSeverity } ); } - M.define( 'skins.minerva.scripts/cleanuptemplates', { + M.define( 'skins.minerva.scripts/pageIssues', { init: initPageIssues, // The logger requires initialization (subscription). Ideally, the logger would be initialized // and passed to initPageIssues() by the client. Since it's not, expose a log method and hide // the subscription call in cleanuptemplates. - log: pageIssueLogger.log, + log: pageIssuesLogger.log, test: { createBanner: createBanner } diff --git a/resources/skins.minerva.scripts/pageIssueLogger.js b/resources/skins.minerva.scripts/pageIssuesLogger.js similarity index 98% rename from resources/skins.minerva.scripts/pageIssueLogger.js rename to resources/skins.minerva.scripts/pageIssuesLogger.js index 5cea5c4..4a6db0b 100644 --- a/resources/skins.minerva.scripts/pageIssueLogger.js +++ b/resources/skins.minerva.scripts/pageIssuesLogger.js @@ -89,7 +89,7 @@ mwTrack( EVENT_PAGE_ISSUE_LOG, data ); } - M.define( 'skins.minerva.scripts/pageIssueLogger', { + M.define( 'skins.minerva.scripts/pageIssuesLogger', { newPageIssueSchemaData: newPageIssueSchemaData, subscribe: subscribe, log: log diff --git a/resources/skins.minerva.scripts/pageIssueParser.js b/resources/skins.minerva.scripts/pageIssuesParser.js similarity index 98% rename from resources/skins.minerva.scripts/pageIssueParser.js rename to resources/skins.minerva.scripts/pageIssuesParser.js index d924715..01f23ab 100644 --- a/resources/skins.minerva.scripts/pageIssueParser.js +++ b/resources/skins.minerva.scripts/pageIssuesParser.js @@ -156,7 +156,7 @@ /** * @module skins.minerva.scripts/utils */ - M.define( 'skins.minerva.scripts/pageIssueParser', { + M.define( 'skins.minerva.scripts/pageIssuesParser', { /** * Extract an icon for use with the issue. * @param {JQuery.Object} $box element to extract the icon from diff --git a/skin.json b/skin.json index 4d10eff..5234d80 100644 --- a/skin.json +++ b/skin.json @@ -393,7 +393,6 @@ "mediawiki.Title", "mobile.startup", "skins.minerva.mainMenu", - "mobile.issues", "mobile.search.api", "mobile.search", "mobile.references", @@ -416,15 +415,20 @@ "mobile-frontend-redirected-from" ], "styles": [ - "resources/skins.minerva.scripts/styles.less" + "resources/skins.minerva.scripts/styles.less", + "resources/skins.minerva.scripts/PageIssuesOverlay.less" ], + "templates": { + "PageIssuesOverlayContent.hogan": "resources/skins.minerva.scripts/PageIssuesOverlayContent.hogan" + }, "scripts": [ "resources/skins.minerva.scripts/preInit.js", "resources/skins.minerva.scripts/DownloadIcon.js", - "resources/skins.minerva.scripts/pageIssueLogger.js", - "resources/skins.minerva.scripts/pageIssueParser.js", + "resources/skins.minerva.scripts/pageIssuesLogger.js", + "resources/skins.minerva.scripts/pageIssuesParser.js", "resources/skins.minerva.scripts/AB.js", - "resources/skins.minerva.scripts/cleanuptemplates.js", + "resources/skins.minerva.scripts/PageIssuesOverlay.js", + "resources/skins.minerva.scripts/pageIssues.js", "resources/skins.minerva.scripts/init.js", "resources/skins.minerva.scripts/initLogging.js", "resources/skins.minerva.scripts/mobileRedirect.js", diff --git a/tests/qunit/skins.minerva.scripts/stubs.js b/tests/qunit/skins.minerva.scripts/stubs.js new file mode 100644 index 0000000..47bec9d --- /dev/null +++ b/tests/qunit/skins.minerva.scripts/stubs.js @@ -0,0 +1 @@ +mw.template.add( 'skins.minerva.scripts', 'PageIssuesOverlayContent.hogan', '' ); diff --git a/tests/qunit/skins.minerva.scripts/test_cleanuptemplates.js b/tests/qunit/skins.minerva.scripts/test_pageIssues.js similarity index 94% rename from tests/qunit/skins.minerva.scripts/test_cleanuptemplates.js rename to tests/qunit/skins.minerva.scripts/test_pageIssues.js index be92ea6..4f83341 100644 --- a/tests/qunit/skins.minerva.scripts/test_cleanuptemplates.js +++ b/tests/qunit/skins.minerva.scripts/test_pageIssues.js @@ -1,5 +1,5 @@ ( function ( M ) { - var createBanner = M.require( 'skins.minerva.scripts/cleanuptemplates' ).test.createBanner, + var createBanner = M.require( 'skins.minerva.scripts/pageIssues' ).test.createBanner, OverlayManager = M.require( 'mobile.startup/OverlayManager' ), Page = M.require( 'mobile.startup/Page' ), overlayManager = new OverlayManager( require( 'mediawiki.router' ) ), diff --git a/tests/qunit/skins.minerva.scripts/test_pageIssueParser.js b/tests/qunit/skins.minerva.scripts/test_pageIssuesParser.js similarity index 91% rename from tests/qunit/skins.minerva.scripts/test_pageIssueParser.js rename to tests/qunit/skins.minerva.scripts/test_pageIssuesParser.js index 36d71b9..0b020f0 100644 --- a/tests/qunit/skins.minerva.scripts/test_pageIssueParser.js +++ b/tests/qunit/skins.minerva.scripts/test_pageIssuesParser.js @@ -1,7 +1,7 @@ ( function ( M ) { - var pageIssueParser = M.require( 'skins.minerva.scripts/pageIssueParser' ); + var pageIssuesParser = M.require( 'skins.minerva.scripts/pageIssuesParser' ); - QUnit.module( 'Minerva pageIssueParser' ); + QUnit.module( 'Minerva pageIssuesParser' ); /** * @param {string} className @@ -34,7 +34,7 @@ test = params[2], box = newBox( className ); assert.strictEqual( - pageIssueParser.test.parseSeverity( box ), + pageIssuesParser.test.parseSeverity( box ), expect, 'Result should be the correct severity; case ' + i + ': ' + test + '.' ); @@ -63,7 +63,7 @@ test = params[3], box = newBox( className ); assert.propEqual( - pageIssueParser.test.parseType( box, severity ), + pageIssuesParser.test.parseType( box, severity ), expect, 'Result should be the correct icon type; case ' + i + ': ' + test + '.' ); @@ -90,7 +90,7 @@ expect = params[2], box = newBox( className ); assert.strictEqual( - pageIssueParser.iconName( box, severity ), + pageIssuesParser.iconName( box, severity ), expect, 'Result should be the correct ResourceLoader icon name; case ' + i + ': ' + severity + '.' ); @@ -112,7 +112,7 @@ expect = params[1]; assert.strictEqual( - pageIssueParser.maxSeverity( severities ), + pageIssuesParser.maxSeverity( severities ), expect, 'Result should be the highest severity in the array; case ' + i + '.' );