Page MenuHomePhabricator

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

Description

Progress is measured here: mediawiki.org

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.

How?

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: https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/468187/14/src/mobile.startup/loadingOverlay.js

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.

Outcomes

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"

Icons

Drawers and Panels

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

Categories

MediaViewer

  • ImageOverlay extends Overlay -> is Overlay (T216198)

Languages

  • LanguageOverlay extends Overlay -> is Overlay

Notifications

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

Page issues

Talk

  • 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

Editor

  • AbuseFilterOverlay extends Overlay -> is Overlay
  • VisualEditorOverlay extends EditorOverlayBase extends Overlay -> is Overlay
  • EditorOverlayBase extends Overlay -> devnull
  • EditorOverlay extends EditorOverlayBase -> is Overlay

Misc

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

e.g.

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

Talk:
https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/471334/7/src/mobile.talk.overlays/talkOverlay.js

Sign off steps

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

Related Objects

StatusAssignedTask
OpenNone
ResolvedJdlrobson
OpenNone
OpenNone
ResolvedNone
ResolvedJdlrobson
ResolvedNone
ResolvedNiedzielski
Resolvedovasileva
OpenJdlrobson
ResolvedJdlrobson
Resolvedphuedx
OpenNone
Resolvedovasileva
DeclinedJdlrobson
Resolvednray
StalledNone
ResolvedNone
ResolvedNone
OpenNone

Event Timeline

Jdlrobson triaged this task as High priority.Dec 20 2018, 10:38 PM
Jdlrobson created this task.
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)Dec 20 2018, 10:41 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)Dec 20 2018, 10:44 PM
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)
Jdlrobson updated the task description. (Show Details)Jan 10 2019, 12:26 AM
Jdlrobson updated the task description. (Show Details)Jan 10 2019, 8:10 PM
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 updated the task description. (Show Details)Jan 16 2019, 4:06 PM
Jdlrobson updated the task description. (Show Details)Jan 24 2019, 10:41 PM
Jdlrobson moved this task from Backlog to Tech debt on the MinervaNeue board.Feb 5 2019, 6:50 PM
Jdlrobson updated the task description. (Show Details)Feb 11 2019, 6:00 PM
Jdlrobson updated the task description. (Show Details)Feb 11 2019, 11:24 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 updated the task description. (Show Details)Feb 26 2019, 12:28 AM
Jdlrobson raised the priority of this task from High to Needs Triage.Feb 26 2019, 7:09 PM
Jdlrobson updated the task description. (Show Details)Feb 27 2019, 9:24 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)Feb 27 2019, 9:26 PM
Jdlrobson updated the task description. (Show Details)Feb 27 2019, 9:39 PM
Jdlrobson triaged this task as High priority.Feb 27 2019, 10:36 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)Feb 27 2019, 10:54 PM
phuedx removed a subscriber: phuedx.Feb 28 2019, 10:58 AM
Jdlrobson updated the task description. (Show Details)Mar 7 2019, 12:48 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)Apr 1 2019, 10:12 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)Jul 24 2019, 12:52 AM
Jdlrobson updated the task description. (Show Details)Fri, Aug 2, 2:15 AM
Jdlrobson updated the task description. (Show Details)Wed, Aug 7, 3:24 AM