From 3dc9cff2c2398a9d35e6c2ec113e680040d05173 Mon Sep 17 00:00:00 2001 From: Stephen Niedzielski Date: Wed, 23 Jan 2019 13:16:11 -0700 Subject: [PATCH] Hygiene: separate page issue view logic - Move page issue view components that do not modify the DOM during during construction to PageIssueLearnMoreLink.js and PageIssueLink.js. PascalCase is used optimistically for filenaming in the hopes that these functions can become something like a JSX component. A "new" function prefix is used in the meantime. - Move page issue view logic that munges the existing DOM to pageIssueFormatter.js. Substitute "create" prefixes for insert so that clients won't forget that calling the function is a modify operation. Alternative naming welcome but it shouldn't be confused with more idealistic components that do not depend on DOM state for construction. - Consolidate createPageIssueBanner() and createPageIssueBannerMultiple() into insertPageIssueBanner() as the code was quite similar and were it a true component, it would probably be a single component. All new files appear under page/ to keep their distinction from the overlay code clear. Some view logic remains in pageIssues.js but it shall be difficult to isolate. Bug: T212376 Change-Id: Iccce709c34fa8de5a28a5a00098add5775e3dc9a --- includes/MinervaHooks.php | 3 + .../page/PageIssueLearnMoreLink.js | 14 +++ .../page-issues/page/PageIssueLink.js | 13 +++ .../page-issues/page/pageIssueFormatter.js | 51 +++++++++ resources/skins.minerva.scripts/pageIssues.js | 107 ++---------------- skin.json | 3 + .../skins.minerva.scripts/pageIssues.test.js | 10 +- 7 files changed, 100 insertions(+), 101 deletions(-) create mode 100644 resources/skins.minerva.scripts/page-issues/page/PageIssueLearnMoreLink.js create mode 100644 resources/skins.minerva.scripts/page-issues/page/PageIssueLink.js create mode 100644 resources/skins.minerva.scripts/page-issues/page/pageIssueFormatter.js diff --git a/includes/MinervaHooks.php b/includes/MinervaHooks.php index 42b2536..183574a 100644 --- a/includes/MinervaHooks.php +++ b/includes/MinervaHooks.php @@ -111,6 +111,9 @@ class MinervaHooks { 'resources/skins.minerva.scripts/page-issues/overlay/IssueNotice.js', 'resources/skins.minerva.scripts/page-issues/overlay/IssueList.js', 'resources/skins.minerva.scripts/page-issues/overlay/pageIssuesOverlay.js', + "resources/skins.minerva.scripts/page-issues/page/PageIssueLearnMoreLink.js", + "resources/skins.minerva.scripts/page-issues/page/PageIssueLink.js", + "resources/skins.minerva.scripts/page-issues/page/pageIssueFormatter.js", 'resources/skins.minerva.scripts/pageIssues.js', // test files 'tests/qunit/skins.minerva.scripts/downloadPageAction.test.js', diff --git a/resources/skins.minerva.scripts/page-issues/page/PageIssueLearnMoreLink.js b/resources/skins.minerva.scripts/page-issues/page/PageIssueLearnMoreLink.js new file mode 100644 index 0000000..72fc07c --- /dev/null +++ b/resources/skins.minerva.scripts/page-issues/page/PageIssueLearnMoreLink.js @@ -0,0 +1,14 @@ +( function ( M ) { + /** + * Creates a "read more" button with given text. + * @param {string} msg + * @return {JQuery} + */ + function newPageIssueLearnMoreLink( msg ) { + return $( '' ) + .addClass( 'ambox-learn-more' ) + .text( msg ); + } + + M.define( 'skins.minerva.scripts/page-issues/page/PageIssueLearnMoreLink', newPageIssueLearnMoreLink ); +}( mw.mobileFrontend ) ); diff --git a/resources/skins.minerva.scripts/page-issues/page/PageIssueLink.js b/resources/skins.minerva.scripts/page-issues/page/PageIssueLink.js new file mode 100644 index 0000000..679ba8f --- /dev/null +++ b/resources/skins.minerva.scripts/page-issues/page/PageIssueLink.js @@ -0,0 +1,13 @@ +( function ( M ) { + /** + * Create a link element that opens the issues overlay. + * + * @param {string} labelText The text value of the element + * @return {JQuery} + */ + function newPageIssueLink( labelText ) { + return $( '' ).addClass( 'cleanup mw-mf-cleanup' ).text( labelText ); + } + + M.define( 'skins.minerva.scripts/page-issues/page/PageIssueLink', newPageIssueLink ); +}( mw.mobileFrontend ) ); diff --git a/resources/skins.minerva.scripts/page-issues/page/pageIssueFormatter.js b/resources/skins.minerva.scripts/page-issues/page/pageIssueFormatter.js new file mode 100644 index 0000000..0173bb5 --- /dev/null +++ b/resources/skins.minerva.scripts/page-issues/page/pageIssueFormatter.js @@ -0,0 +1,51 @@ +( function ( M ) { + var + newPageIssueLink = M.require( 'skins.minerva.scripts/page-issues/page/PageIssueLink' ), + newPageIssueLearnMoreLink = M.require( 'skins.minerva.scripts/page-issues/page/PageIssueLearnMoreLink' ); + + /** + * Modifies the `issue` DOM to create a banner designed for single / multiple issue templates, + * and handles event-binding for that issues overlay. + * + * @param {IssueSummary} issue + * @param {string} msg + * @param {string} overlayUrl + * @param {Object} overlayManager + * @param {boolean} [multiple] + */ + function insertPageIssueBanner( issue, msg, overlayUrl, overlayManager, multiple ) { + var $learnMoreEl = newPageIssueLearnMoreLink( msg ), + $issueContainer = multiple ? + issue.$el.parents( '.mbox-text-span, .mbox-text-div' ) : + issue.$el.find( '.mbox-text' ), + $clickContainer = multiple ? issue.$el.parents( '.mbox-text' ) : issue.$el; + + $issueContainer.prepend( issue.iconString ); + $issueContainer.prepend( $learnMoreEl ); + + $clickContainer.on( 'click', function () { + overlayManager.router.navigate( overlayUrl ); + return false; + } ); + } + + /** + * Modifies the page DOM to insert a page-issue notice below the title of the page, + * containing a link with a message like "this page has issues". + * Used on talk & category namespaces, or when page-issue banners have been disabled.s + * + * @param {string} labelText + * @param {string} section + */ + function insertPageIssueNotice( labelText, section ) { + var $link = newPageIssueLink( labelText ); + $link.attr( 'href', '#/issues/' + section ); + // eslint-disable-next-line jquery/no-global-selector + $link.insertAfter( $( 'h1#section_0' ) ); + } + + M.define( 'skins.minerva.scripts/page-issues/page/pageIssueFormatter', { + insertPageIssueBanner: insertPageIssueBanner, + insertPageIssueNotice: insertPageIssueNotice + } ); +}( mw.mobileFrontend ) ); diff --git a/resources/skins.minerva.scripts/pageIssues.js b/resources/skins.minerva.scripts/pageIssues.js index 0fc7f34..f2559bf 100644 --- a/resources/skins.minerva.scripts/pageIssues.js +++ b/resources/skins.minerva.scripts/pageIssues.js @@ -10,94 +10,12 @@ features = mw.config.get( 'wgMinervaFeatures', {} ), pageIssuesParser = M.require( 'skins.minerva.scripts/pageIssuesParser' ), pageIssuesOverlay = M.require( 'skins.minerva.scripts/pageIssuesOverlay' ), + pageIssueFormatter = M.require( 'skins.minerva.scripts/page-issues/page/pageIssueFormatter' ), // When the query string flag is set force on new treatment. // When wgMinervaPageIssuesNewTreatment is the default this line can be removed. QUERY_STRING_FLAG = mw.util.getParamValue( 'minerva-issues' ), newTreatmentEnabled = features.pageIssues || QUERY_STRING_FLAG; - /** - * Create a link element that opens the issues overlay. - * - * @ignore - * - * @param {string} labelText The text value of the element - * @return {JQuery} - */ - function createLinkElement( labelText ) { - return $( '' ).addClass( 'cleanup mw-mf-cleanup' ).text( labelText ); - } - - /** - * Creates a "read more" button with given text. - * @param {string} msg - * @return {JQuery} - */ - function createLearnMoreLink( msg ) { - return $( '' ) - .addClass( 'ambox-learn-more' ) - .text( msg ); - } - - /** - * Modifies the `issue` DOM to create a banner designed for - * single issue templates, and handles event-binding for that issues overlay. - * - * @param {IssueSummary} issue - * @param {string} msg - * @param {string} overlayUrl - * @param {Object} overlayManager - */ - function createPageIssueBanner( issue, msg, overlayUrl, overlayManager ) { - var $learnMoreEl = createLearnMoreLink( msg ), - $issueContainer = issue.$el.find( '.mbox-text' ); - - $issueContainer.prepend( issue.iconString ); - $issueContainer.prepend( $learnMoreEl ); - - issue.$el.on( 'click', function () { - overlayManager.router.navigate( overlayUrl ); - return false; - } ); - } - - /** - * Modifies the `issue` DOM to create a banner designed for - * multiple issue templates, and handles event-binding for that issues overlay. - * - * @param {IssueSummary} issue - * @param {string} msg - * @param {string} overlayUrl - * @param {Object} overlayManager - */ - function createPageIssueBannerMultiple( issue, msg, overlayUrl, overlayManager ) { - var $learnMoreEl = createLearnMoreLink( msg ), - $parentContentContainer = issue.$el.parents( '.mbox-text-span, .mbox-text-div' ), - $parentContainer = issue.$el.parents( '.mbox-text' ); - - $parentContentContainer.prepend( issue.iconString ); - $parentContentContainer.prepend( $learnMoreEl ); - - $parentContainer.on( 'click', function () { - overlayManager.router.navigate( overlayUrl ); - return false; - } ); - } - - /** - * Modifies the page DOM to insert a page-issue notice below the title of the page, - * containing a link with a message like "this page has issues". - * Used on talk & category namespaces, or when page-issue banners have been disabled.s - * - * @param {string} labelText - * @param {string} section - */ - function createPageIssueNotice( labelText, section ) { - var $link = createLinkElement( labelText ); - $link.attr( 'href', '#/issues/' + section ); - // eslint-disable-next-line jquery/no-global-selector - $link.insertAfter( $( 'h1#section_0' ) ); - } - /** * Render a banner in a containing element. * if in group B, a learn more link will be append to any amboxes inside $container @@ -117,7 +35,7 @@ * * @return {JQuery.Object} */ - function createBanner( page, labelText, section, inline, overlayManager ) { + function insertBannersOrNotice( page, labelText, section, inline, overlayManager ) { var $metadata, issueUrl = section === KEYWORD_ALL_SECTIONS ? '#/issues/' + KEYWORD_ALL_SECTIONS : '#/issues/' + section, @@ -153,16 +71,13 @@ issueSummaries.forEach( function ( issueSummary, i ) { var isGrouped = issueSummary.issue.grouped, lastIssueIsGrouped = issueSummaries[ i - 1 ] && - issueSummaries[ i - 1 ].issue.grouped; + issueSummaries[ i - 1 ].issue.grouped, + multiple = isGrouped && !lastIssueIsGrouped; // only render the first grouped issue of each group - if ( isGrouped && !lastIssueIsGrouped ) { - createPageIssueBannerMultiple( issueSummary, mw.msg( 'skin-minerva-issue-learn-more' ), issueUrl, overlayManager ); - } else { - createPageIssueBanner( issueSummary, mw.msg( 'skin-minerva-issue-learn-more' ), issueUrl, overlayManager ); - } + pageIssueFormatter.insertPageIssueBanner( issueSummary, mw.msg( 'skin-minerva-issue-learn-more' ), issueUrl, overlayManager, multiple ); } ); } else if ( issueSummaries.length ) { - createPageIssueNotice( labelText, section ); + pageIssueFormatter.insertPageIssueNotice( labelText, section ); } return $metadata; @@ -236,15 +151,15 @@ if ( CURRENT_NS === NS_TALK || CURRENT_NS === NS_CATEGORY ) { // e.g. Template:English variant category; Template:WikiProject - createBanner( page, mw.msg( 'mobile-frontend-meta-data-issues-header-talk' ), + insertBannersOrNotice( page, mw.msg( 'mobile-frontend-meta-data-issues-header-talk' ), KEYWORD_ALL_SECTIONS, inline, overlayManager ); } else if ( CURRENT_NS === NS_MAIN ) { label = mw.msg( 'mobile-frontend-meta-data-issues-header' ); if ( issueOverlayShowAll ) { - createBanner( page, label, KEYWORD_ALL_SECTIONS, inline, overlayManager ); + insertBannersOrNotice( page, label, KEYWORD_ALL_SECTIONS, inline, overlayManager ); } else { // parse lead - createBanner( page, label, '0', inline, overlayManager ); + insertBannersOrNotice( page, label, '0', inline, overlayManager ); if ( newTreatmentEnabled ) { // parse other sections but only in group B. In treatment A no issues are shown // for sections. @@ -257,7 +172,7 @@ if ( sectionNum ) { // Render banner for sectionNum associated with headingEl inside // Page - createBanner( + insertBannersOrNotice( page, label, sectionNum.toString(), inline, overlayManager ); } @@ -278,7 +193,7 @@ init: initPageIssues, test: { getAllIssuesSections: getAllIssuesSections, - createBanner: createBanner + insertBannersOrNotice: insertBannersOrNotice } } ); diff --git a/skin.json b/skin.json index 1c70258..844fbcb 100644 --- a/skin.json +++ b/skin.json @@ -418,6 +418,9 @@ "resources/skins.minerva.scripts/page-issues/overlay/IssueNotice.js", "resources/skins.minerva.scripts/page-issues/overlay/IssueList.js", "resources/skins.minerva.scripts/page-issues/overlay/pageIssuesOverlay.js", + "resources/skins.minerva.scripts/page-issues/page/PageIssueLearnMoreLink.js", + "resources/skins.minerva.scripts/page-issues/page/PageIssueLink.js", + "resources/skins.minerva.scripts/page-issues/page/pageIssueFormatter.js", "resources/skins.minerva.scripts/pageIssues.js", "resources/skins.minerva.scripts/init.js", "resources/skins.minerva.scripts/initLogging.js", diff --git a/tests/qunit/skins.minerva.scripts/pageIssues.test.js b/tests/qunit/skins.minerva.scripts/pageIssues.test.js index 8e60ed0..6ebfa2b 100644 --- a/tests/qunit/skins.minerva.scripts/pageIssues.test.js +++ b/tests/qunit/skins.minerva.scripts/pageIssues.test.js @@ -3,7 +3,7 @@ pageIssues = M.require( 'skins.minerva.scripts/pageIssues' ), mobile = M.require( 'mobile.startup' ), util = mobile.util, - createBanner = pageIssues.test.createBanner, + insertBannersOrNotice = pageIssues.test.insertBannersOrNotice, icon = {}, MEDIUM_ISSUE = { issue: { @@ -45,21 +45,21 @@ labelText = 'label text', inline = true, SECTION = '0', - processedAmbox = createBanner( + processedAmbox = insertBannersOrNotice( new Page( { el: $mockContainer } ), labelText, SECTION, inline, overlayManager ); QUnit.module( 'Minerva cleanuptemplates' ); - QUnit.test( 'createBanner() should add a "learn more" message', function ( assert ) { + QUnit.test( 'insertBannersOrNotice() should add a "learn more" message', function ( assert ) { assert.strictEqual( /⧼skin-minerva-issue-learn-more⧽/.test( processedAmbox.html() ), true ); } ); - QUnit.test( 'createBanner() should add an icon', function ( assert ) { + QUnit.test( 'insertBannersOrNotice() should add an icon', function ( assert ) { assert.strictEqual( /mw-ui-icon/.test( processedAmbox.html() ), true ); } ); - QUnit.test( 'clicking on the product of createBanner() should trigger a URL change', function ( assert ) { + QUnit.test( 'clicking on the product of insertBannersOrNotice() should trigger a URL change', function ( assert ) { processedAmbox.click(); assert.strictEqual( window.location.hash, '#/issues/' + SECTION ); } );