Page MenuHomePhabricator

Extract Echo handling from SkinMinerva class
Open, LowPublic

Description

SkinMinerva class has some logic related to the Echo extension (useEcho(), getEchoNotifUser(), getEchoSeenTime(), getFormattedEchoNotificationCount()). Instead of hardcoding Echo-related logic inside Skin class, please provide:

  • an interface that defines contract between Skin and Notifications system
  • a concrete class that provides Notifications data when Echo is not avaialble
  • a concrete class that provides Notifications data when Echo is avaialble (maybe put that in Echo extension ??)
  • register proper concrete definition in the ServiceWirings file. The SkinMinerva class should depend only on one service Minerva.NotificationsProvider, and should not know about the Echo extensions.

Developer notes

This code would ideally live in Echo, and be registered via hook. By default where Echo is not installed we show a link to the user's talk page. Echo would be able to replace that button with the Notifications link and register the code.

Event Timeline

pmiazga created this task.Apr 15 2019, 3:52 PM
Restricted Application added a project: Growth-Team. · View Herald TranscriptApr 15 2019, 3:52 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a subscriber: Jdlrobson.

Ideally this code should exist in Echo. The contract as far as I'm concerned should be similar to other menus - the top right corner should be a menu with 1 item (for now - the user talk page link) and Echo's job would be to replace that menu item with the notification button.

ovasileva triaged this task as Low priority.May 28 2019, 4:36 PM
ovasileva added a project: good first bug.
ovasileva moved this task from Inbox to Triaged but Future on the Growth-Team board.

If someone decides to pick up this task I'm happy to guide him through the required changes.

ovasileva moved this task from Triaged but Future to Inbox on the Growth-Team board.

@MMiller_WMF and @ovasileva will coordinate to determine the best way forward on this task.

@Catrope -- could you please give us a t-shirt size estimate for this work? A day, a week, a month? We're trying to figure out whether/when/how to do this.

More than one day, probably close to a week. This also requires a combination of doing some thinking about how exactly to approach this, and getting a brain dump from Piotr.

Thanks, @Catrope. I'm putting this in Upcoming Work as a potential task to put in a lighter sprint.

Change 540223 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Remove Echo code from MobileFrontend

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

Change 540224 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Echo@master] WIP: Mobile notifications overlay lives in Echo

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

Change 540225 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Prepare for move of Echo code to Echo extension from MobileFrontend and Minerva

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

Change 540226 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Goodbye Echo code from Minerva

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

Change 540226 abandoned by Jdlrobson:
Goodbye Echo code from Minerva

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

Hey @kostajh and @Catrope !

Okay, I've taken a look at this and have managed to get all the mobile code in Echo.
There is one Minerva and one Echo change.

The Echo change needs to be merged first:
https://gerrit.wikimedia.org/r/#/q/I09c27a084100b223662f84de6cbe01bebe1fe774
I've basically moved all the code from MobileFrontend and Minerva to Echo and tidied it up a little to make it ES5 and limit the use of MobileFrontend's code where possible. I'm happy to clean this up some more if there's anything you don't like and you have the time to work through that with me.

This code in Echo lays dormant until the second patch in Minerva lands which exposes a VERSION property that Echo's frontend code is watching out for which spurs it into action:
https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/540225/

The new relationship will be as follows:

  1. Minerva will render a bell icon pointing to Special:MyTalk and expose a hook that allows replacement of that button
  2. Echo registers to that hook and replaces the Notification badge
  3. An echo.mobile module is used to register the NotificationsOverlay with the MobileFrontend OverlayManager.
  4. Minerva listens to a client side hook that Echo evokes to handle the drawer-effect.

Both changes are blocked on an issue with extension schema in core to allow packageFiles and templates in tests:
https://gerrit.wikimedia.org/r/#/q/I1a66939d2b596094b419de40b370e79f09c85581

I think there's a lot more we can do following these changes - in particular I'd love to explore unifying the Vector bell with Minerva's bell as well as exploring with some designers whether the Vector treatment can be used in Minerva (and if not document why). Now they are in the same codebase this should be much easier.

I am happy to talk through the patch with either/both of you. I have left some guiding comments on my patch that hopefully help them to make more sense. Let me know!

Change 541648 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Echo@master] No longer check for skins.minerva.scripts

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

Niharika removed a subscriber: Niharika.Wed, Oct 9, 2:34 AM

Change 540224 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Dormant mobile notifications overlay lives in Echo

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

Change 540225 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Echo is removed from Minerva

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

Change 540223 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove Echo code from MobileFrontend

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

Restricted Application added a subscriber: Masumrezarock100. · View Herald TranscriptSat, Oct 12, 8:19 AM

Working with the growth team this code has now been moved to the Echo extension and is easier to work with. Adding to sprint board for visibility.

Change 541648 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] No longer check for skins.minerva.scripts

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

Change 543398 had a related patch set uploaded (by Zoranzoki21; owner: Zoranzoki21):
[mediawiki/extensions/Echo@master] Remove comment related to skins.minerva.scripts after removal of it

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

Change 543398 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Remove comment related to skins.minerva.scripts after removal of it in 1053997

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