Page MenuHomePhabricator
Create Task
Maniphest T221007

Extract Echo handling from SkinMinerva class
Closed, ResolvedPublic
Actions

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.

Details

Related Gerrit Patches:
mediawiki/skins/MinervaNeue : masterRemove unused test class
mediawiki/skins/MinervaNeue : masterRemove `Echo` phan config as MobileFrontend doesn't depend upon Echo
mediawiki/extensions/Echo : masterDormant mobile notifications overlay lives in Echo
mediawiki/extensions/Echo : masterRemove comment related to skins.minerva.scripts after removal of it in 1053997
mediawiki/extensions/Echo : masterNo longer check for skins.minerva.scripts
mediawiki/extensions/MobileFrontend : masterRemove Echo code from MobileFrontend
mediawiki/skins/MinervaNeue : masterEcho is removed from Minerva
mediawiki/skins/MinervaNeue : masterGoodbye Echo code from Minerva

Related Objects
Search...

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 moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.Apr 23 2019, 7:50 AM
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.
pmiazga added a comment.May 28 2019, 4:37 PM

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

ovasileva moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.May 28 2019, 4:38 PM
ovasileva moved this task from Triaged but Future to Inbox on the Growth-Team board.
JTannerWMF moved this task from Inbox to Needs Discussion/Analysis on the Growth-Team board.May 28 2019, 5:55 PM
JTannerWMF added subscribers: MMiller_WMF, ovasileva, JTannerWMF.

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

MMiller_WMF added a subscriber: Catrope.May 29 2019, 9:00 PM

@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.

Catrope added a comment.Jun 5 2019, 12:15 AM

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.

MMiller_WMF moved this task from Needs Discussion/Analysis to Upcoming Work on the Growth-Team board.Jun 7 2019, 8:45 PM

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

gerritbot added a comment.Oct 1 2019, 10:01 PM

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

gerritbot added a project: Patch-For-Review.Oct 1 2019, 10:01 PM

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

gerritbot added a comment.Oct 1 2019, 10:06 PM

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

gerritbot added a comment.Oct 1 2019, 10:06 PM

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

gerritbot added a comment.Oct 3 2019, 9:23 PM

Change 540226 abandoned by Jdlrobson:
Goodbye Echo code from Minerva

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

Jdlrobson added a subscriber: kostajh.Oct 3 2019, 11:17 PM

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!

gerritbot added a comment.Oct 8 2019, 9:42 PM

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
gerritbot added a comment.Oct 9 2019, 7:50 PM

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

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

gerritbot added a comment.Oct 9 2019, 7:51 PM

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

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

gerritbot added a comment.Oct 12 2019, 8:19 AM

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
Jdlrobson edited projects, added Readers-Web-Backlog (Kanbanana-2019-20-Q2); removed Readers-Web-Backlog (Tracking).Oct 15 2019, 4:54 PM
Jdlrobson moved this task from Needs Analysis to Ready for Signoff on the Readers-Web-Backlog (Kanbanana-2019-20-Q2) board.

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.

gerritbot added a comment.Oct 16 2019, 10:22 AM

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

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

gerritbot added a comment.Oct 16 2019, 10:28 AM

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

gerritbot added a comment.Oct 16 2019, 11:02 AM

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
pmiazga added a comment.Oct 22 2019, 3:30 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
pmiazga moved this task from Ready for Signoff to Needs More Work on the Readers-Web-Backlog (Kanbanana-2019-20-Q2) board.
gerritbot added a comment.Oct 22 2019, 7:00 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

Jdlrobson added a comment.Oct 22 2019, 7:01 PM
  • 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..

gerritbot added a comment.Oct 22 2019, 9:09 PM

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

gerritbot added a comment.Oct 22 2019, 9:35 PM

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

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

gerritbot added a comment.Oct 22 2019, 9:35 PM

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)
Content licensed under Creative Commons Attribution-ShareAlike 3.0 (CC-BY-SA) unless otherwise noted; code licensed under GNU General Public License (GPL) or other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL