User:JDrewniak (WMF)/notes/The issues with PageIssues js

Responsibilities

1. Define an initialization module for entire page-issues feature. Currently this means:

 M.define( 'skins.minerva.scripts/pageIssues', {
    init: initPageIssues,
    test: {
        getAllIssuesSections: getAllIssuesSections,
        createBanner: createBanner
    }
} );

The initialization function, initPageIssues( [overlay, page] ) and essentially the whole feature, does the following:

• identifies pages-issues on the page
• parses their severities
• adds classes & icons to style them accordingly
• attaches events that open the overlay
  • when clicking on the top page-issues, overlay should display all issues
  • when clicking on section issues, only display issues from that section
• adds a 'read more' button.

This is the feature in a nutshell, (minus instrumentation). The functionality should work accordingly for the following contexts:

• only work on the main namespace, for the following scenarios:
• single issues at the top of the page
• groups of issues at the top of the page
• single issues in sections
• group of issues in sections

The majority of effort has been made to parse the enwiki page-issue markup (ambox) templates, given that many wikis use that as a base for their templates.

CreateLinkElement

Creates a link element.

function createLinkElement( labelText ) {
    return $( '<a class="cleanup mw-mf-cleanup"></a>' ) .text( labelText ); 
}

createBanner

Given a page, create (i.e. invoke) all the page-issues features, in-place, on a single (or not!) DOM element, populate a global allIssues variable, and return the modified page-issues elements (or remove them!).

Code walkthrough

• GIVEN
  • page,(object) labelText,(string) section,(number) inline (boolean - the new or old treatment?), overlayManager (object)
• DEFINE
  • the overlay URL fragment
  • the element CSS selectors
  • an empty array of issues
• IF we're working with issues at the top of the page:
  • select all the issues on the page ( var $metaData),
  • otherwise select the issues in the section
• remove something called .NavFrame from that selection
• LOOP through each element in the selection
  • IF the element contains exactly zero child elements that match the selector, and has text:
    • create a PageIssue data model of that element using pageIssuesParser.extract
    • add that PageIssue to the array of issues.
• POPULATE allIssues with the issues for this section.
• IF there are issues, and inline (new treatment) is true
  • APPEND and icon to the first issue
  • CREATE a "learn more" button
  • IF this is a "multiple issues" template, insert the button in one place
  • ELSE insert the button in a different place
  • ATTACH the click handlers
• ELSE
  • createLinkElement() and insert that "this page has issues" link at the top of the page.
  • remove the selected issues from the DOM (var $metaData)
• RETURN selected issues (var $metaData)

getIssues

Given a page section(number) or KEYWORD_ALL_SECTIONS(string), returns the value of that section from allIssues array. Hopefully that array exists in the outer scope and is populated.

Code walkthrough

• GIVEN section
• IF section is NOT KEYWORD_ALL_SECTIONS:
  • RETURN all issues for that section OR empty array
• IF allIssues.all exists
  • RETURN allIssues.all
• ELSE RETURN an array containing all the key values of allIssues

If minerva is run in desktop mode, it expects the property allIssues.all to exist, and returns that value. Where is allIssues.all set?

getAllIssuesSections

Given an array (or is it a map?) called allIssues , flips that map and returns a flat array of numbers representing the section of each issue on the page. If that issue is part of a group, (i.e. multiple issues template) it only returns one section value for that group.

Code walkthrough

• GIVEN allIssues:
• RETURN REDUCE: allIssues ( [], section)
  • IF section has issues
    • LOOP through each issue
      • IF issue is .grouped
      • AND the last issue is .grouped
      • AND the last issues has already been added to []
        • set the last value in [] to the current issue section
      • ELSE
        • push the section number into []

Do we need to set the last value in [] to the current issue section? Shouldn't that value already be set?

initPageIssues

Given overlayManager and page, creates the page-issue banners depending on the namespace and whether or not the new page-issues treatment has been enabled.

Code walkthrough

• GIVEN ovevrlayManager, page
• DEFINE
  • $lead with page.getLeadSectionElement() I think this is a proxy for telling whether or not we're in desktop mode or not?
  • issueOverlayShowAll if we're in category or talk namespace, or there is no $lead
  • inline if new treatment is enabled and we're in the main namespace.
• IF newTreatmentEnabled add a 'issues-group-B class to the element.
• IF we're in the talk or category namespace:
  • createBanner with a mw.msg for talk pages, KEYWORD_ALL_SECTIONS, inline (false)
• ELSE IF we're in the main namespace:
  • SET label to a mw.msg for main pages "this page has issues"
  • IF issueOverlayShowAll we're in desktop mode
    • createBanner() with label , KEYWORD_ALL_SECTIONS, inline (true if new treatment enabled)
  • ELSE we're in mobile mode
    • createBanner() with page, label, section "0", inline
    • IF newTreatmentEnabled
      • FIND Page.HEADING_SELECTOR elements in page page
      • FOREACH heading element
        • FIND a section number in that heading element
        • IF section number exists
          • createBanner with that section number
• add a route to overlayManager for section numbers and all issues overlay
  • when accessed, return new PageIssuesOverlay for that section and getIssues( section )

Suggestions

Mise en place! we should gather required variables & DOM elements up-front so that we don't have to do these operations as a side-effect of other functions.

1. allIssues seems like an important variable used to represent all page-issues on a given page. It's currently populated as a side-effect of createBanner(). It should be populated before calling this function, OR, issues could be a property of Page.
2. createBanner() shouldn't have to loop through DOM nodes to find issues, this process should happen before createBanner is called, and createdBanner should be given a data-model of the page-issues instead (.i.e, allIssues). This would ease testing as well.
3. createBanner()is responsible for creating both the new banner and the old banner. This could be separated into separate "createNewBanner" and "createOldBanner" functions (but with a better names).
4. Both createBanner() and initPageIssues() do multiple checks for old vs new treatment. CreateBanner wouldn't have to make this check if was split into create "old banner" and "new banner" functions.
5. initPageIssues() contains deeply nested conditionals, which make the function hard to reason about. The logic boils down to "what namespace are we on?" "is the new treatment enabled?" "are we in desktop mode?" and "should the overlay show all issues?". These questions could be answered with their own function. Also, these questions should be answered before calling createBanner.
6. initPageIssues() calls createBanner() 4 times. Instead, it could assemble the required parameters, and call createBanner() once at the end of the function.