Page MenuHomePhabricator

[EPIC] None of our View's should exhibit 2 levels of inheritance
Open, HighPublic


Progress is measured here:

This begins our refactoring effort and aims to minimise the use of inheritance in MobileFrontend. We will reduce components to classes that extend the View class to minimise confusion in navigating through files to understand how the inheritance change impacts our component.

Per T206036#4832740 (MFA: [Spike, 8hrs] Views: You Gotta Keep 'em Separated) and similar discussions in T209647 our main focus will be the Overlay's. We will limit efforts to separating the content of overlays from the overlay itself.


We can replace many of our classes with factory functions that create Overlays and append child components via jQuery.
The LoadingOverlay probably provides the most simple to explain example:

We will look to generate many of our View's in this way, resorting to using existing components with different options (think React-like props). Where needed, we will add functionality/flexibility to View and Overlay classes to generalise different component needs - for example in the LoadingOverlay example a will described property noHeader was added.


The outcomes of this epic will be:

  • More granularity in our components and more reuse
  • The DRYing up of many template and template partials
  • Easier to read code.
  • The ability to remove the LoadingOverlay - instead, Overlay factories will be able to limit the spinner/async loader indicator to the Overlay content
  • We have documented a best practice for how to make new overlay's / components going forward

When is it done?

This EPIC can be resolved when the following classes either do not exist (replaced with factory functions or removed)

  1. Documentation on how to make View's has been written
  2. The following query should return 0 results in Minerva and MobileFrontend.

ag "extends Overlay"
ag "extends Drawer"
ag "extends Icon"
ag "extends PageList"


Drawers and Panels

  • BetaOptinPanel extends Panel -> extends View T217298
  • BlockMessage extends Drawer -> is Drawer
  • Drawer extends Panel -> is Panel
  • CtaDrawer extends Drawer extends Panel -> is Drawer (red links + watchstar when anon!)
  • ReferencesDrawer extends Drawer -> is Drawer (T217295)



  • ImageOverlay extends Overlay -> is Overlay (T216198)


  • LanguageOverlay extends Overlay -> is Overlay


  • NotificationsFilterOverlay extends Overlay -> is Overlay (T217296)
  • NotificationsOverlay extends Overlay -> is Overlay (T217296)

Page issues


  • TalkOverlay extends Overlay (T215370)
  • TalkSectionAddOverlay extends Overlay (T217102)
  • TalkSectionOverlay extends Overlay (T217810)

Page lists

  • SearchOverlay extends Overlay
  • WatchstarPageList extends PageList -> is PageList
  • Nearby extends WatchstarPageList extends PageList (T217814)
  • WatchList extends WatchstarPageList extends PageList -> is PageList


  • AbuseFilterOverlay extends Overlay -> is Overlay
  • VisualEditorOverlay extends EditorOverlayBase extends Overlay -> is Overlay
  • EditorOverlayBase extends Overlay -> devnull
  • EditorOverlay extends EditorOverlayBase -> is Overlay
  • AbuseFilterPanel extends View -> is Panel



Whether we want to do this will depend on the future of the lazy references feature...

  • ReferencesHtmlScraperGateway extends ReferencesGateway
  • ReferencesMobileViewGateway extends ReferencesHtmlScraperGateway

What happens after this epic?

We'll review the newly created components and with a spike evaluate how we can break them down further.
As part of this we may decide to target one specific overlay and refactor until completion to show/prove to ourselves this will result in more manageable and testable code.

Do we write new tests?

This will be decided by the team as and when we work on this. We'll talk about our commitment to code coverage and how it will change during this refactor.

What will code look like in future?

All Overlay's will be Overlay's with widgets inside them


var overlay = new Overlay( {
  title: 'Editing Spain article'
} );
  new EditorSurface()


Sign off steps

  • Take a look at T214641 and determine whether the issue has been solved. If not adjust/rewrite

Related Objects

Resolved Jdlrobson
Open Jdlrobson
Resolved Jdlrobson
Declined Jdlrobson

Event Timeline

Jdlrobson triaged this task as High priority.
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
Jdlrobson renamed this task from [EPIC] Nothing should extend the Overlay class to [EPIC] None of our View's should exhibit 2 levels of inheritance.Dec 20 2018, 11:23 PM
Jdlrobson updated the task description. (Show Details)
nray awarded a token.Dec 21 2018, 1:39 AM
Jdlrobson updated the task description. (Show Details)Jan 3 2019, 4:32 PM
Jdlrobson updated the task description. (Show Details)Jan 4 2019, 5:24 PM
Jdlrobson updated the task description. (Show Details)
phuedx added a subscriber: phuedx.Jan 16 2019, 2:39 PM

Can I ask that an outcome of this epic be well-written documentation about how to author new overlays both within and without the MobileFrontend/MinervaNeue codebases?

Jdlrobson moved this task from Backlog to Tech debt on the MinervaNeue board.Feb 5 2019, 6:50 PM

Do we want to add any requirements around mfExtend()? I think this is mostly used on Views but it's also used in some surprising places like Skin, ScrollEndEventEmitter, and ReferencesMobileViewGateway. It seems to me that it should only be used for second layer Views.

It might be nice if at least View, and perhaps other inheritance pattern usages, forbid modification via a deep Object.freeze(). This would make it easier to limit and reason about the View API, and prevent unintentional changes to the prototypal hierarchy by child classes.

Do we want to keep mfExtend? Would using ES6 classes make it redundant (and remove the need for the oojs dependency from mobile.startup/ the critical path)

I would love to drop mfExtend!!! I haven't looked into the OO internals so I'm not sure how practical it will be to drop but we could make that our goal and see where we end up?

Jdlrobson raised the priority of this task from High to Needs Triage.
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
Jdlrobson triaged this task as High priority.Feb 27 2019, 10:36 PM
Jdlrobson updated the task description. (Show Details)
phuedx removed a subscriber: phuedx.Feb 28 2019, 10:58 AM
Jdlrobson updated the task description. (Show Details)Mar 7 2019, 1:02 AM
Jdlrobson updated the task description. (Show Details)Mar 7 2019, 1:13 AM
Jdlrobson updated the task description. (Show Details)