golem
/
MinervaNeue
9
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

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
This commit is contained in:
Stephen Niedzielski 2019-01-23 13:16:11 -07:00 committed by Niedzielski
parent 4b7aabca6f
commit 3dc9cff2c2
7 changed files with 100 additions and 101 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
includes/MinervaHooks.php
View File

@ -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',

14
resources/skins.minerva.scripts/page-issues/page/PageIssueLearnMoreLink.js Normal file
View File

@ -0,0 +1,14 @@
( function ( M ) {
/**
* Creates a "read more" button with given text.
* @param {string} msg
* @return {JQuery}
*/
function newPageIssueLearnMoreLink( msg ) {
return $( '<span>' )
.addClass( 'ambox-learn-more' )
.text( msg );
}
M.define( 'skins.minerva.scripts/page-issues/page/PageIssueLearnMoreLink', newPageIssueLearnMoreLink );
}( mw.mobileFrontend ) );

13
resources/skins.minerva.scripts/page-issues/page/PageIssueLink.js Normal file
View File

@ -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 $( '<a>' ).addClass( 'cleanup mw-mf-cleanup' ).text( labelText );
}
M.define( 'skins.minerva.scripts/page-issues/page/PageIssueLink', newPageIssueLink );
}( mw.mobileFrontend ) );

51
resources/skins.minerva.scripts/page-issues/page/pageIssueFormatter.js Normal file
View File

@ -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 ) );

107
resources/skins.minerva.scripts/pageIssues.js
View File

@ -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 $( '<a>' ).addClass( 'cleanup mw-mf-cleanup' ).text( labelText );
}
/**
* Creates a "read more" button with given text.
* @param {string} msg
* @return {JQuery}
*/
function createLearnMoreLink( msg ) {
return $( '<span>' )
.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
}
} );

3
skin.json
View File

@ -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",

10
tests/qunit/skins.minerva.scripts/pageIssues.test.js
View File

@ -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 );
} );