Page MenuHomePhabricator

MFA: View: Make isBorderBox and className passable options to the View class to reduce reliance on inheritance
Closed, ResolvedPublic3 Story Points

Description

To change the class of a View (or to add the isBorderBox class), you need to extend the View class and set a className property.

This is a bit inconvenient for simple components.

A quick scan of the codebase (MobileFrontend only) shows we do this in approx 33 places.

Motivations

Doing this, will simplify a lot of the logic of our Views and help us reduce View's to template, model/gateway code. For the latter, we'll want to move that out (off the back of T206036) and this will help give us clarity of what's going on where.

Acceptance criteria

  • View's can pass className as an option.
  • View's can pass isBorderBox as an option.
  • Test coverage is added for the new options
  • Port at least 3 existing classes over to the new method to prove its working (ideally we'd port all usages and remove the properties on View, but that would probably increase the scope unnecessarily large - providing the foundation to chip away at this is the important bit!)

Example:

Before:

	function TableOfContents( props ) {
		View.call( this, props);
	}

	OO.mfExtend( TableOfContents, View, {
		className: 'toc-mobile'

After:

	function TableOfContents( props ) {
		View.call( this, util.extend( props, {
			className: 'toc-mobile'
		} ) );
	}
	OO.mfExtend( TableOfContents, View, {

[1]

ag className: src/ resources/ | wc -l

Developer notes

An example for className (with tests) is shown here: https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/468185

However isBorderBox will need to be done separately.

Related Objects

StatusAssignedTask
OpenNone
ResolvedJdlrobson
OpenNone
DeclinedNone
DeclinedNone
OpenNone
OpenNone
ResolvedKrinkle
Resolvedovasileva
ResolvedJdlrobson
OpenNone
ResolvedNone
ResolvedNone
Resolved Niedzielski
Resolvedovasileva
OpenJdlrobson
ResolvedJdlrobson
Resolvedphuedx
ResolvedJdlrobson
Resolvedovasileva
DeclinedJdlrobson
Resolvednray
StalledNone
ResolvedNone
ResolvedNone
OpenNone
OpenNone
OpenNone
DuplicateNone
OpenNone
Resolvedpmiazga
ResolvedJdlrobson
DeclinedNone
ResolvedJdlrobson
ResolvedJdlrobson
DeclinedNone
Resolved Niedzielski
DuplicateReedy
Resolved Niedzielski
ResolvedJdlrobson
ResolvedJdlrobson
DuplicateNone
Resolvedpmiazga
Resolvedovasileva
Resolvedovasileva
Resolvedovasileva
Resolved Niedzielski
OpenNone
Resolved Niedzielski

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 7 2018, 11:04 PM
Jdlrobson triaged this task as High priority.Nov 7 2018, 11:04 PM
Jdlrobson updated the task description. (Show Details)

Change 468185 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Composition: The className of a View can be set directly by a property

https://gerrit.wikimedia.org/r/468185

Jdlrobson updated the task description. (Show Details)Nov 7 2018, 11:08 PM
Jdlrobson moved this task from Triaged but Future to Upcoming on the Readers-Web-Backlog board.
Jdlrobson renamed this task from View: Make isTemplateMode, isBorderBox and className passable options to the View class to reduce reliance on inheritance to View: Make isBorderBox and className passable options to the View class to reduce reliance on inheritance.Nov 7 2018, 11:19 PM
Jdlrobson updated the task description. (Show Details)

@ovasileva this task is triaged as high, and has a aptch written by @Jdlrobson - should we bring it into Readers-Web-Kanbanana-Board-2018-19-Q2 ?

@pmiazga no rush and patch is incomplete. Estimation and discussion needed. We shouldn't be pulling stuff into sprint without those!

Jdlrobson set the point value for this task to 3.Nov 13 2018, 5:41 PM

We estimated a 2 and a couple of 3s. The View constructor is invoked in all View's so there is a small risk here since a change here could potentially impact all views. This feel more risky than say a test change e.g. T209007

nray added a subscriber: nray.

Moving this to Needs More work. One patch has been reviewed and merge relating to class name but there is still AC to be met including passing isBorderBox

Change 468185 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Composition: The className of a View can be set directly by a property

https://gerrit.wikimedia.org/r/468185

Jdlrobson updated the task description. (Show Details)Nov 15 2018, 5:54 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson renamed this task from View: Make isBorderBox and className passable options to the View class to reduce reliance on inheritance to MFA: View: Make isBorderBox and className passable options to the View class to reduce reliance on inheritance.Nov 15 2018, 6:58 PM
Jdlrobson updated the task description. (Show Details)Nov 15 2018, 7:39 PM

Change 473829 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Several overlays switched to use className options

https://gerrit.wikimedia.org/r/473829

Just poking around the Views & Overlays, I discovered the View uses either props.className or this.className in _postInitialize() (View.js#220):

_postInitialize: function ( props ) {
	this.$el.addClass( props.className || this.className );
	if ( this.isBorderBox ) {
		// FIXME: Merge with className property (?)
		this.$el.addClass( 'view-border-box' );
	}
	this.render( this.options );
}

That means that when we're calling Overlay (which extends View) with this, i.e. Overlay.call( this, options ), the following two approaches are still equivalent.

function SearchOverlay( options ) {
	Overlay.call( this,
			util.extend( options, {
			className: 'overlay search-overlay'
		 } )
	);
	this.api = options.api;
	this.gateway = new options.gatewayClass( this.api );
	this.router = options.router;
}
function SearchOverlay( options ) {
	this.api = options.api;
	this.gateway = new options.gatewayClass( this.api );
	this.router = options.router;
	this.className = 'overlay search-overlay';
	Overlay.call( this,  options );
}

So I guess I'm wondering, if we want to pass in the className explicitly, do we want to remove the reference to this.className from _postInitialize() (View.js#220) as well?

Change 472349 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Composition: TableOfContents is View w/ options + templates

https://gerrit.wikimedia.org/r/472349

Change 475146 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] isBorderBox should now be passed in as an option

https://gerrit.wikimedia.org/r/475146

Change 475148 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Officially deprecate isBorderBox and className when not using option

https://gerrit.wikimedia.org/r/475148

st poking around the Views & Overlays, I discovered the View uses either props.className or this.className in _postInitialize() (View.js#220):

Yep. That's new and part of this patch (see https://gerrit.wikimedia.org/r/468185)

So I guess I'm wondering, if we want to pass in the className explicitly, do we want to remove the reference to this.className from _postInitialize() (View.js#220) as well?

Long term yes, short term, no. The this.className is needed as most of our classes are still using className and we'll need to migrate them over to use the new method. I have submitted a patch to explicitly mark these properties as deprecated to begin that process:

https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/475148

Change 475147 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] View's now pass isBorderBox as a property

https://gerrit.wikimedia.org/r/475147

Change 473829 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Several overlays switched to use className options

https://gerrit.wikimedia.org/r/473829

There was a bug in the code merged in T209007#4766466 that @Jdrewniak found and fixed. Given the merge happened on a Wednesday this didn't make it into the deploy branch.. phew.

Change 475146 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] isBorderBox should now be passed in as an option

https://gerrit.wikimedia.org/r/475146

Change 475354 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] isBorderBox and className properties are deprecated

https://gerrit.wikimedia.org/r/475354

Change 475354 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] isBorderBox and className properties are deprecated

https://gerrit.wikimedia.org/r/475354

Change 475147 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Several View's now pass className and isBorderBox as a property

https://gerrit.wikimedia.org/r/475147

Esanders added a subscriber: Esanders.EditedNov 29 2018, 12:28 PM

We have a regression in VE where the editor-overlay-ve class isn't getting applied: T210640, is this related?

Fixed locally with a rebuild, and beta cluster appears to have been fixed too.

Jdlrobson updated the task description. (Show Details)Nov 29 2018, 7:06 PM

This work is handed over to @Jdrewniak and @nray capable hands.
I will be available for code changes prior to 12pm PST, after which I will not be interacting with this card.
@MBinder_WMF I trust you to make sure it doesn't break the 2 week rule and linger in the sprint board. :)

Thanks, @Jdlrobson . FWIW, this task has already broken that rule, but I can track it for 2 weeks more if that's useful. :)

Change 476905 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Remove deprecation warnings for className usage

https://gerrit.wikimedia.org/r/476905

Handing this over to Jan.

Change 475148 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Officially deprecate isBorderBox and className when not using option

https://gerrit.wikimedia.org/r/475148

Change 476905 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove deprecation warnings for className usage

https://gerrit.wikimedia.org/r/476905

Jdlrobson closed this task as Resolved.Dec 12 2018, 10:34 PM

The warning Use of "className" is deprecated. Setting className on the View is deprecated. Please use options. is showing correctly so this is working. T211828 will follow up on that.

All the workflows seem to appear and work correctly.

Great teamwork here all. This proved to be a tough one!

Jdlrobson updated the task description. (Show Details)Dec 12 2018, 10:34 PM