Page MenuHomePhabricator

Popups render method should be dumb
Closed, ResolvedPublic

Description

Currently the renderer for Popups mixes view and controller logic.

  1. Code in ext.popups.core.js calls a render method
  2. The mw.popups.render.render method makes an api request via a call to mw.popups.render.renderers.article.init
  3. When this is completed it makes a call to mw.popups.render.openPopup
  4. The mw.track method is called in [[ URL | various ]] places by the renderer - a render cannot be called without triggering an event:
  5. resources/ext.popups.renderer.desktopRenderer/desktopRenderer.js,
  6. resources/ext.popups.desktop/ext.popups.renderer.article.js
  7. resources/ext.popups.desktop/ext.popups.settings.js
  8. resources/ext.popups.targets.desktopTarget/desktopTarget.js

This is problematic as it means our rendering functions have side effects.
I propose that we separate rendering logic from event logging logic.

On inspection it seems like the ext.popups.core or ext.popups.renderer.desktopRenderer would be a good place to keep this logic however ext.popups.renderer.desktopRenderer mixes initialisation of Hovercards and events with rendering

Suggested changes:

  1. Logic in mw.popups.render.render should be moved to a new PopupsGateway class. Hopefully this method can be entirely removed.
  2. The openPopup method should be repurposed as a showPopup method which is passed data from the PopupsGateway class. It should not have to think about caching or know anything about the link that triggered it
  3. In ext.popups.core.js the gateway should be used to retrieve the data for the hovercard and this should be passed to the openPopup method and rendered.
  4. Introduce a PopupsController class that dispatches to render methods and event logging methods when necessary.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 22 2016, 10:45 PM
Jdlrobson updated the task description. (Show Details)Oct 20 2016, 8:01 PM
bmansurov triaged this task as Normal priority.Oct 26 2016, 8:26 PM
bmansurov edited projects, added Technical-Debt (RW-Tech-Debt); removed Technical-Debt.
bmansurov moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.

Most of it is obsolete, probably. Needs analysis.