Page MenuHomePhabricator

Extract pageIssues.js view code
Closed, ResolvedPublic3 Estimated Story Points

Description

pageIssues.js has too many responsibilities. This task will remove one by identifying view and presentation logic and moving it to distinct files separate from both pageIssues.js and the related but completely distinct pageIssuesOverlay.js.


	/**
	 * 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 class="cleanup mw-mf-cleanup"></a>' )
			.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.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.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 );
		$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
	 * if in group A or control, any amboxes in container will be removed and a link "page issues"
	 * will be rendered above the heading.
	 * This function comes with side effects. It will populate a global "allIssues" object which
	 * will link section numbers to issues.
	 * @param {Page} page to search for page issues inside
	 * @param {string} labelText what the label of the page issues banner should say
	 * @param {string} section that the banner and its issues belong to.
	 *  If string KEYWORD_ALL_SECTIONS banner should apply to entire page.
	 * @param {boolean} inline - if true the first ambox in the section will become the entry point
	 *                           for the issues overlay
	 *  and if false, a link will be rendered under the heading.
	 * @param {OverlayManager} overlayManager
	 * @ignore
	 *
	 * @return {JQuery.Object}
	 */
	function createBanner( page, labelText, section, inline, overlayManager ) {
		var
			$metadata,
			issueUrl = section === KEYWORD_ALL_SECTIONS ? '#/issues/' + KEYWORD_ALL_SECTIONS : '#/issues/' + section,
			selector = 'table.ambox, table.tmbox, table.cmbox, table.fmbox',
			issueSummaries = [];

		if ( section === KEYWORD_ALL_SECTIONS ) {
			$metadata = page.$( selector );
		} else {
			// find heading associated with the section
			$metadata = page.findChildInSectionLead( parseInt( section, 10 ), selector );
		}
		// clean it up a little
		$metadata.find( '.NavFrame' ).remove();
		$metadata.each( function () {
			var issueSummary,
				$this = $( this );

			if ( $this.find( selector ).length === 0 ) {
				issueSummary = pageIssuesParser.extract( $this );
				// Some issues after "extract" has been run will have no text.
				// For example in Template:Talk header the table will be removed and no issue found.
				// These should not be rendered.
				if ( issueSummary.text ) {
					issueSummaries.push( issueSummary );
				}
			}
		} );
		// store it for later
		allIssues[section] = issueSummaries;

		if ( inline ) {
			issueSummaries.forEach( function ( issueSummary, i ) {
				var isGrouped = issueSummary.issue.grouped,
					lastIssueIsGrouped = issueSummaries[ i - 1] &&
						issueSummaries[ i - 1].issue.grouped;
				// 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 );
				}
			} );
		} else if ( issueSummaries.length ) {
			createPageIssueNotice( labelText, section );
		}

		return $metadata;
	}

	/**
	 * Obtains the list of issues for the current page and provided section
	 * @param {number|string} section either KEYWORD_ALL_SECTIONS or a number relating to the
	 *                                section the issues belong to
	 * @return {jQuery.Object[]} array of all issues.
	 */
	function getIssues( section ) {
		if ( section !== KEYWORD_ALL_SECTIONS ) {
			return allIssues[section] || [];
		}
		// Note section.all may not exist, depending on the structure of the HTML page.
		// It will only exist when Minerva has been run in desktop mode.
		// If it's absent, we'll reduce all the other lists into one.
		return allIssues.all || Object.keys( allIssues ).reduce(
			function ( all, key ) {
				return all.concat( allIssues[key] );
			},
			[]
		);
	}

Acceptance criteria

  • Code that creates or binds to views is moved to new *View.js file(s)
    • Views are generated from plain models that are independent of the DOM and Views
  • Code that reformats existing DOM Elements is moved to new *Formatter.js file(s)

QA steps

Do a general QA for page issues with the following pages:

The risk of issues is low, so center testing on the display and opening of the page issues overlay.

QA Results

StatusDetails))
✅ PassedT212376#4970780

Event Timeline

Restricted Application changed the subtype of this task from "Deadline" to "Task". · View Herald TranscriptDec 19 2018, 11:53 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

There seems like a lot of overlap here with T212371.
Some sample code of where we want to be would help distinguish the different scoped responsibilities of this task and the other one!

This task is partly to identify view logic within the scope of pageIssues.js. However, createLinkElement(), createLearnMoreLink(), createPageIssueBanner(), createPageIssueBannerMultiple(), createPageIssueNotice() in the provided snippet can all be moved directly. createBanner() may needs some refactoring.

@Niedzielski are you saying all of those functions would live in their own files?
It would help to have a strawman proposal of the expected directory tree at the end of this, so it's clear what the scope is. Can you add that in a developer note at your leisure?

Niedzielski renamed this task from Extract pageIssues.js view code to [Analysis] Extract pageIssues.js view code.Jan 14 2019, 6:14 PM

Change 486152 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] Hygiene: separate page issue view logic

https://gerrit.wikimedia.org/r/486152

I've tagged an example patch that I hope is more illustrative. In my view, this is ready to work when priorities permit.

Niedzielski renamed this task from [Analysis] Extract pageIssues.js view code to Extract pageIssues.js view code.Jan 23 2019, 8:25 PM
Jdlrobson moved this task from Bugs to Tech debt on the MinervaNeue board.
Jdlrobson set the point value for this task to 3.Feb 7 2019, 4:40 PM

patch looks uncontroversial and cleans things up, but 3 because we're as always working with user generated content. We did an async estimation and mostly arrived on this number.

I reviewed the code today as it looked good and I was a little concerned if left any longer it would rot.

Change 486152 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Hygiene: separate page issue view logic

https://gerrit.wikimedia.org/r/486152

Test Result

Status: ✅ PASS
OS: macOS Mojave, iOS 12.1.4
Browser: Chrome DevTools Device Emulator (iPhone X, iPad, Galaxy S5), Safari on iOS

Test Artifact(s):

Screen Shot 2019-02-20 at 10.59.08 PM.png (630×760 px, 86 KB)

Screen Shot 2019-02-20 at 10.59.50 PM.png (1×770 px, 234 KB)

Screen Shot 2019-02-20 at 10.59.23 PM.png (164×756 px, 17 KB)

Screen Shot 2019-02-20 at 10.59.34 PM.png (512×758 px, 75 KB)

Screen Shot 2019-02-20 at 11.00.19 PM.png (1×768 px, 440 KB)

Screen Shot 2019-02-20 at 11.00.33 PM.png (638×764 px, 90 KB)