Page MenuHomePhabricator

MFA: Remove CtaDrawer events and pass events for red link CtaDrawer via options
Closed, ResolvedPublic5 Story Points

Description

This task is to remove CtaDrawer.events and make it a pass in option instead. This may require some changes to View.js like the recent border box changes. Add tests to make sure that passing the option has the same effect as the current class property.

We almost had a regression in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/476576/ due to an event defined inside the CtaDrawer class which appears to only live to support a link in Minerva in resources/skins.minerva.scripts/init.js

The code in Minerva looks like this:

var drawerOptions = {
                progressiveButton: new Button( {
                        progressive: true,
                        label: mw.msg( 'mobile-frontend-editor-redlink-create' ),
                        href: $( this ).attr( 'href' )
                } ).options,
                closeAnchor: new Anchor( {
                        progressive: true,
                        label: mw.msg( 'mobile-frontend-editor-redlink-leave' ),
                        additionalClassNames: 'hide'
                } ).options,
                content: mw.msg( 'mobile-frontend-editor-redlink-explain' ),
                actionAnchor: false
        },
        drawer = new CtaDrawer( drawerOptions );

Acceptance criteria

  • CtaDrawer does not define any events
  • The code in Minerva has been updated to pass the hide event as a property

Developer notes

Update to View's

One possible solution would be to allow events to be passed as a property (and bound to this) by a change to Views' such as https://gerrit.wikimedia.org/r/468188

                        var drawerOptions = {
                                        progressiveButton: new Button( {
                                                progressive: true,
                                                label: mw.msg( 'mobile-frontend-editor-redlink-create' ),
                                                href: $( this ).attr( 'href' )
                                        } ).options,
                                        closeAnchor: new Anchor( {
                                                progressive: true,
                                                events: {
	                                                	'click .hide': onHide
	                                         },
                                                label: mw.msg( 'mobile-frontend-editor-redlink-leave' ),
                                                additionalClassNames: 'hide'
                                        } ).options,
                                        content: mw.msg( 'mobile-frontend-editor-redlink-explain' ),
                                        actionAnchor: false
                                },
                                drawer = new CtaDrawer( drawerOptions );

Another would be to extend the CtaDrawer class inside Minerva.

Any other options?

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 30 2018, 6:59 PM
Jdlrobson renamed this task from Remove CtaDrawer.prototype.hide function to Remove CtaDrawer events and pass events for red link CtaDrawer via options.Nov 30 2018, 7:00 PM
Jdlrobson triaged this task as High priority.
Jdlrobson moved this task from Triaged but Future to Upcoming on the Readers-Web-Backlog board.
Jdlrobson set the point value for this task to 5.Dec 11 2018, 5:14 PM
phuedx added a subscriber: phuedx.Dec 11 2018, 5:15 PM

I can conceive of a situation where you'd define a view with some default click event handler but an extending class wants to define its own click handler. In this instance, I'd expect both event handlers to be bound rather than one. Is this the intended behaviour?

Niedzielski updated the task description. (Show Details)Dec 11 2018, 5:24 PM

I can conceive of a situation where you'd define a view with some default click event handler but an extending class wants to define its own click handler. In this instance, I'd expect both event handlers to be bound rather than one. Is this the intended behaviour?

I don't think we need any magic in View to handle this. I'd expect all functions to be bound to the current class, so the extending class can use this to call

e.g.

Pooh.prototype = {
events: { 'click': 'hide'}
};
new Pooh( {
  events: {
    click: function () {
      alert('Silly old bear!');
      this.hide();
    }
  }
});
Jdlrobson renamed this task from Remove CtaDrawer events and pass events for red link CtaDrawer via options to MFA: Remove CtaDrawer events and pass events for red link CtaDrawer via options.Dec 12 2018, 6:25 PM
Niedzielski added a subscriber: Niedzielski.

The context genie has smiled upon me!

Change 481007 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: deprecate View.events and update CtaDrawer

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

Change 481029 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] Hygiene: revise CtaDrawer.events to options

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

Change 481029 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Hygiene: revise CtaDrawer.events to options

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

Change 481007 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: deprecate View.events and update CtaDrawer

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

phuedx removed a subscriber: phuedx.Dec 21 2018, 3:57 PM
Jdlrobson closed this task as Resolved.Dec 21 2018, 11:05 PM
Jdlrobson updated the task description. (Show Details)

Signing this off.