Page MenuHomePhabricator

Extract Echo handling from SkinMinerva class
Closed, ResolvedPublic

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 move the echo code into the Echo extension.

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 task.
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.Oct 9 2019, 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 TranscriptOct 12 2019, 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

pmiazga claimed this task.Oct 17 2019, 4:26 PM

The patches above tackled moving the Echo stuff out of Minerva skin (moved related notifications code to Echo extension). There is no defined contract between Minerva and Echo, only a not documented hook SkinMinervaReplaceNotificationsBadge. This approach is also valid but before I can signoff this task we need to:

  • document the SkinMinervaReplaceNotificationsBadge hook
  • remove Echo from .phan directory_list config as it's not required any more.
  • removed unused EchoNotifUser class from SkinMinervaTest
  • the mobile-frontend-user-newmessages in qqq.json says Tooltip for new messages on the users talk page (if no Extension:Echo installed).", now it can be anything other than Echo, would be nice to update this message
  • resources/skins.minerva.scripts/menu.js still has some Echo handling. Do we want to move it? It uses hooks, so most probably can stay, I just wanted to highlight this.
pmiazga removed pmiazga as the assignee of this task.Oct 22 2019, 3:31 PM

Change 545374 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Remove unused test class

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

  • document the SkinMinervaReplaceNotificationsBadge hook

https://www.mediawiki.org/wiki/Skin:Minerva_Neue/SkinMinervaReplaceNotificationsBadge?venotify=created

  • remove Echo from .phan directory_list config as it's not required any more.

Could you clarify what this means or submit a patch? Cannot see what you are referring to.

  • removed unused EchoNotifUser class from SkinMinervaTest

Done in https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/545374

  • the mobile-frontend-user-newmessages in qqq.json says Tooltip for new messages on the users talk page (if no Extension:Echo installed).", now it can be anything other than Echo, would be nice to update this message

I think the message is still accurate. Echo is and should be the only thing that uses this. If I don't add this comment, it will be unclear to translators when this might happen.

  • resources/skins.minerva.scripts/menu.js still has some Echo handling. Do we want to move it? It uses hooks, so most probably can stay, I just wanted to highlight this.

It does yes. I've talked about removing this as part of T226125 - this code will sadly have to stick around until that happens or we make our main menu work without JavaScript and decouple it from the notifications overlay T225213..

Change 545394 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/skins/MinervaNeue@master] Remove Echo phan config as MobileFrontend doesn't depend upon Echo

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

Change 545374 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Remove unused test class

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

Change 545394 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Remove Echo phan config as MobileFrontend doesn't depend upon Echo

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

pmiazga closed this task as Resolved.Oct 24 2019, 2:24 PM
pmiazga updated the task description. (Show Details)