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

= Responsibilities =

The initialization function,  and essentially the whole feature, does the following:
 * 1) Define an initialization module for entire page-issues feature. Currently this means:


 * 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.

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

Code walkthrough


 * GIVEN
 * ,(object) ,(string)  ,(number)   (boolean - the new or old treatment?),   (object)
 * DEFINE
 * the overlay URL fragment
 * the element CSS
 * an empty array of
 * 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  from that selection
 * LOOP through each element in the selection
 * IF the element contains exactly zero child elements that match the, and has text:
 * create a PageIssue data model of that element using
 * add that PageIssue to the array of.
 * POPULATE  with the   for this.
 * IF there are, and   (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
 * 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 (number) or  (string), returns the value of that section from   array. Hopefully that array exists in the outer scope and is populated.

Code walkthrough


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

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

getAllIssuesSections
Given an array (or is it a map?) called , 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 :
 * RETURN REDUCE:
 * IF  has issues
 * LOOP through each issue
 * IF issue is
 * AND the last issue is
 * 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  and , creates the page-issue banners depending on the namespace and whether or not the new page-issues treatment has been enabled.

Code walkthrough


 * GIVEN ,
 * DEFINE
 * with  I think this is a proxy for telling whether or not we're in desktop mode or not?
 * if we're in category or talk namespace, or there is no
 * if new treatment is enabled and we're in the main namespace.
 * IF  add a   class to the element.
 * IF we're in the talk or category namespace:
 * with a mw.msg for talk pages,,   (false)
 * ELSE IF we're in the main namespace:
 * SET  to a mw.msg for main pages "this page has issues"
 * IF  we're in desktop mode
 * with ,  ,   (true if new treatment enabled)
 * ELSE we're in mobile mode
 * with,  , section "0",
 * IF
 * FIND  elements in page
 * FOREACH heading element
 * FIND a section number in that heading element
 * IF section number exists
 * with that section number
 * add a route to  for section numbers and all issues overlay
 * when accessed, return new PageIssuesOverlay for that section and

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)   seems like an important variable used to represent all page-issues on a given page. It's currently populated as a side-effect of  . It should be populated before calling this function, OR, issues could be a property of.
 * 2)   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)  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   and   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)   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)   calls   4 times. Instead, it could assemble the required parameters, and call   once at the end of the function.