Page MenuHomePhabricator

Use the Overlay.make pattern for notification feature
Closed, ResolvedPublic8 Story Points

Description

The NotificationOverlay is an example of an Overlay that adds OOjs UI components inside an Overlay, however right now it mixes the two things.

We will pull out a NotificationList component that looks like this:

Acceptance criteria

  • A factory function creates the notification overlay with the header "Notifications" and footer link "All notifications"
  • NotificationsOverlay becomes a NotificationList that extends View
  • The NotificationsOverlay no longer uses the loadingOverlay following the examples of the TalkOverlay and LanguageOverlay

Descoped:

  • Code coverage is added for the newly spun out components (currently it is 0%)

QA steps

Run the following for LTR and RTL displays and on tablet and mobile.

  • Log in
  • Click the icon in the top right of your screen (top left in RTL mode)
  • Expected: A drawer slides in
  • I can close the drawer with back button
  • I can close the drawer by clicking outside it (tablet)
  • I can close the drawer by clicking on the close icon
  • Visit https://en.m.wikipedia.org/wiki/Special:Notifications
  • Click the filter icon
  • Confirm the filter overlay displays and can be closed via close icon or back button
  • Make sure you test the above when you have notifications
  • Make sure you test the above when you do not have notifications (bell icon displays)

To get notifications:

  • Open an incognito window and log in as user B
  • Find an edit made the user A you want to give a notification on Special:Contributions/<username for A>
  • Thank the user A for their edit
  • User A should have a notification.

Sign off steps

  • Ensure tasks are spun out for descoped acceptance criteria.

QA Results

StatusDetails
✅ PassedT217296#5068401

Event Timeline

Jdlrobson renamed this task from Use the Overlay.make pattern inside NotificationOverlay to Use the Overlay.make pattern for notification feature.Feb 27 2019, 10:43 PM
Jdlrobson triaged this task as High priority.
Jdlrobson created this task.
Jdlrobson moved this task from Incoming to Upcoming on the Readers-Web-Backlog board.
phuedx removed a subscriber: phuedx.Feb 28 2019, 10:58 AM
Jdlrobson updated the task description. (Show Details)Mar 5 2019, 5:27 PM
Jdlrobson set the point value for this task to 8.Mar 5 2019, 5:33 PM

Given unfamiliarity with feature and usage of OOjs UI (which we've not tried the overlay pattern with) a pessimistic 8 has been estimated.

Change 495140 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Create the overlay for filters using composition

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

Change 495260 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] WIP: Remove some avoidable inheritance

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

Change 495269 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Hygiene: Drop some unnecessary usages of this.

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

Change 495270 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Use promisedView pattern for dealing with async wrapperWidget

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

Change 495271 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Remove loading checks

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

Change 495272 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] All events handlers are not part of the prototype

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

Change 495273 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] NotificationsOverlay converted to factory function

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

Jdlrobson raised the priority of this task from High to Needs Triage.Mar 8 2019, 8:03 PM
Jdlrobson triaged this task as High priority.Mar 8 2019, 8:36 PM

Change 496175 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Provide a View.make helper method

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

Change 496175 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Provide a View.make helper method

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

Change 495260 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove some avoidable inheritance

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

Change 495269 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: Drop some unnecessary usages of this.

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

Change 497594 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Drop unused parameter

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

Change 495273 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] NotificationsOverlay converted to factory function

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

Change 495270 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Use promisedView pattern for dealing with async wrapperWidget

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

Change 495271 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Remove loading checks

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

Change 497594 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Drop unused parameter in NotificationBadge

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

Change 495271 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove loading checks

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

Change 495272 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] All events handlers are not part of the prototype

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

Change 495140 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Create the overlay for filters using composition

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

Change 497657 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Cleanup NotificationBadge

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

Change 497660 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Make filters overlay using more modern techniques

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

Jdlrobson added a comment.EditedMar 20 2019, 12:09 AM

All patches are now up with the following next in line for reviews (neither blocking each other):
All events handlers are not part of the prototype (MobileFrontend)
Make filters overlay using more modern techniques (Minerva)

Change 495272 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] All events handlers are not part of the prototype

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

Change 495273 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] NotificationsOverlay converted to factory function

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

Change 498409 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Change NotificationOverlay function signature

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

Change 498409 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Minor changes to NotificationOverlay

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

Change 497657 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Cleanup NotificationBadge and notification overlay creation

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

Jdlrobson reassigned this task from Jdlrobson to Edtadros.Mar 22 2019, 9:04 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)Mar 22 2019, 9:06 PM

I just realised there was scope creep here and have spun out the remaining two patches to T219036
Given the struggles we've been having I don't think code coverage is useful right now so I descoped that (let's cut a new task to do that).

Edward please follow the QA steps on https://en.m.wikipedia.beta.wmflabs.org/wiki/Spain

Test Result

Status: ✅ PASS
OS: macOS Mojave
Browser: Chrome DevTools Device Emulator (iPhone X, iPad Pro)

Test Artifact(s): steps

  1. Log in
  2. Click the icon in the top right of your screen (top left in RTL mode)
  3. Expected: A drawer slides in ✅
  4. I can close the drawer with back button ✅
  5. I can close the drawer by clicking outside it (tablet) ✅
  6. I can close the drawer by clicking on the close icon ✅
  7. Visit https://*/wiki/Special:Notifications
  8. Click the filter icon
  9. Confirm the filter overlay displays and can be closed via close icon or back button ✅
  10. Make sure you test the above when you have notifications ✅
  11. Make sure you test the above when you do not have notifications (bell icon displays)
Edtadros updated the task description. (Show Details)Mar 29 2019, 2:07 AM
Edtadros reassigned this task from Edtadros to ovasileva.Mar 29 2019, 2:32 AM
Edtadros added subscribers: ovasileva, Edtadros.

@ovasileva This was tested on the URL that @Jdlrobson provided (thanks!), but the QA Steps state it should also be run in an RTL language. So I'm putting it in needs more work. If you don't need another language this can go to Ready for Signoff.

ovasileva closed this task as Resolved.Mar 29 2019, 12:49 PM

Tested RTL. Signing this off now.