Page MenuHomePhabricator
Create Task
Maniphest T216198

MFA: ImageOverlay should be an Overlay Composed of Components
Closed, ResolvedPublic5 Estimated Story Points
Actions

Assigned To
ovasileva
Authored By
Jdlrobson
Feb 14 2019, 10:57 PM
Tags
Referenced Files
F28513893: T216198-5.png
Mar 30 2019, 1:27 AM
F28513878: T216198-3.png
Mar 30 2019, 1:27 AM
F28513827: T216198-2.png
Mar 30 2019, 1:27 AM
F28513821: T216198-1.png
Mar 30 2019, 1:27 AM
F28513881: T216198-4.png
Mar 30 2019, 1:27 AM
F28513898: T216198-5.png
Mar 30 2019, 1:27 AM
F28247052: Screen Shot 2019-02-19 at 9.48.00 AM.png
Feb 19 2019, 5:50 PM
F28247051: Screen Shot 2019-02-19 at 9.47.26 AM.png
Feb 19 2019, 5:50 PM
View All 9 Files
Subscribers
Aklapper
Edtadros
gerritbot
Jdlrobson
Masumrezarock100
• Niedzielski
• nray

Description

We will remove a level of inheritance and convert the ImageOverlay to an Overlay with a child

Precursors

  • LanguageOverlay should be converted to an Overlay (T215657)
  • Talkoverlay should be an Overlay (T215370)

Acceptance criteria

Anatomy of ImageOverlay

The ImageOverlay has 3 distinct parts. The header/footer probably belong to the Overlay but may require specific styling and themselves may be components!

ImageCarousel

Screen Shot 2019-02-19 at 9.46.23 AM.png (694×703 px, 27 KB)

header

Screen Shot 2019-02-19 at 9.47.26 AM.png (55×1 px, 3 KB)

footer

Screen Shot 2019-02-19 at 9.48.00 AM.png (80×1 px, 9 KB)

Sign off steps

QA steps

This ticket did some refactoring of the overlay but was not intended to change the behavior of the overlay other than its toggling behavior and the removal of the white loading overlay. We are looking for regressions from the production behavior. Device compatibility should also be the same in comparison to production's image overlay ( should work on all Grade A mobile browsers).

The image carousel should let the user see the page's images one-by-one in the order that they appear on the page (from top to bottom). To test, run the following steps:

  • Visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Barack_Obama_T173949
  • Click on one of the images on the page
  • Expected: The black image overlay appears with it eventually loading an image
  • When the image loads, verify that clicking the black part of the overlay (but not the navigation controls) or on the image itself toggles the visibility of the navigation controls and white details bar. Note: this is intentionally differs from production's behavior which also toggles the visibility of the overlay's close icon.
  • When the image loads, test that the back and forward buttons allow you to cycle through the images in the correct order
  • Disconnect your internet connection, click the forward button, and verify that a load error message appears.
  • Turn connection back on, and click the "Refresh" link. Verify that an image successfully loads
  • Close the overlay by clicking on the close icon. Click your browser's forward button and verify that the image you just viewed appears again.

QA Results

StatusDetails
✅ PassedT216198#5071437

Details

SubjectRepoBranchLines +/-
Make minerva use updated mediaViewerOverlay factory functionmediawiki/skins/MinervaNeuemaster+12 -42
Add mediaViewer to mobile.startup exportsmediawiki/extensions/MobileFrontendmaster+15 -11
Make skins.minerva.scripts.init.js use new image overlay factory functionmediawiki/skins/MinervaNeuemaster+27 -31
Create new helper function: makeImagesAndStartIndexmediawiki/extensions/MobileFrontendmaster+203 -2
Refactor ImageOverlay to use new stateful and presentational componentsmediawiki/extensions/MobileFrontendmaster+1 K -808
Add new presentational component: promisedImagemediawiki/extensions/MobileFrontendmaster+187 -2
Add new stateful component: imageCarousel.jsmediawiki/extensions/MobileFrontendmaster+284 -0
Refactor ImageOverlay into an Overlay with an ImageCarouselmediawiki/extensions/MobileFrontendmaster+206 -614
Make minerva exclusively use string emitted by image overlay onSlide eventmediawiki/skins/MinervaNeuemaster+1 -5
Make ImageGateway a required prop for image overlay factory + cleanupmediawiki/extensions/MobileFrontendmaster+16 -29
Improve imageDetails.js APImediawiki/extensions/MobileFrontendmaster+15 -38
Add new presentational component: imageDetails.jsmediawiki/extensions/MobileFrontendmaster+203 -47
Improve carousel APImediawiki/extensions/MobileFrontendmaster+37 -19
Add new presentational component: carousel.jsmediawiki/extensions/MobileFrontendmaster+314 -31
Remove Overlay.prototype.hasFixedHeadermediawiki/extensions/MobileFrontendmaster+63 -21
Let promisedView handle rejects gracefullymediawiki/extensions/MobileFrontendmaster+52 -8
Drop ImageOverlays reliance on template partialsmediawiki/extensions/MobileFrontendmaster+34 -46
Show related patches Customize query in gerrit

Related Objects
Search...

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jdlrobson updated the task description. (Show Details)Feb 19 2019, 5:50 PM
Jdlrobson removed the point value for this task.
Jdlrobson set the point value for this task to 5.

Will this use PromisedView for the overlay to load new content into itself?

I think that's in scope and will hopefully be clearer when T215370 and T215657 are done (I'm not sure whether that will prove easy to do or difficult!)

gerritbot subscribed.Feb 22 2019, 5:25 PM

Change 482382 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Drop ImageOverlays reliance on template partials

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

gerritbot added a project: Patch-For-Review.Feb 22 2019, 5:25 PM
gerritbot added a comment.Feb 22 2019, 5:59 PM

Change 482382 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Drop ImageOverlays reliance on template partials

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

Jdlrobson moved this task from Upcoming to Readers-Web-Kanbanana-Board-2018-19-Q3 on the Web-Team-Backlog board.Feb 25 2019, 6:17 PM
Jdlrobson edited projects, added Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q3); removed Web-Team-Backlog.
Jdlrobson updated the task description. (Show Details)Feb 26 2019, 12:28 AM
Jdlrobson raised the priority of this task from High to Needs Triage.Feb 26 2019, 7:09 PM
Jdlrobson moved this task from Backlog to Review and refactor components on the MobileFrontend (MobileFrontend and MinervaNeue architecture) board.
Jdlrobson triaged this task as High priority.Feb 27 2019, 6:10 PM
• nray claimed this task.Feb 27 2019, 6:30 PM
• nray moved this task from To Do to Doing on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q3) board.
Jdlrobson mentioned this in T212465: [EPIC] None of our View's should exhibit 2 levels of inheritance.Feb 27 2019, 10:54 PM
gerritbot added a comment.Feb 28 2019, 11:44 PM

Change 493630 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] WIP: Refactor ImageOverlay into an Overlay with a ImageCarousel

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

• Jhernandez unsubscribed.Mar 4 2019, 10:07 AM
gerritbot added a comment.Mar 6 2019, 7:12 PM

Change 494809 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Convert Overlay.prototype.hasFixedHeader into prop

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

gerritbot added a comment.Mar 6 2019, 7:54 PM

Change 494820 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Make promisedView.js able to handle promise rejects

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

• nray renamed this task from ImageOverlay should be an Overlay to MFA: ImageOverlay should be an Overlay Composed of Components.Mar 6 2019, 7:56 PM
gerritbot added a comment.Mar 6 2019, 10:37 PM

Change 494820 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Let promisedView handle rejects gracefully

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

phuedx unsubscribed.Mar 7 2019, 11:31 AM
gerritbot added a comment.Mar 8 2019, 12:06 AM

Change 494809 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove Overlay.prototype.hasFixedHeader

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

gerritbot added a comment.Mar 8 2019, 4:20 PM

Change 495248 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Make ImageGateway a required prop for image overlay factory + cleanup

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

gerritbot added a comment.Mar 8 2019, 4:20 PM

Change 495249 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/MinervaNeue@master] Make skins.minerva.scripts.init.js use new image overlay factory function

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

gerritbot added a comment.Mar 12 2019, 3:28 PM

Change 495909 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Add new presentational component: carousel.js

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

gerritbot added a comment.Mar 12 2019, 3:28 PM

Change 495910 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Add new presentational component: promisedImage

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

gerritbot added a comment.Mar 12 2019, 3:28 PM

Change 495911 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Add new presentational component: imageDetails.js

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

gerritbot added a comment.Mar 12 2019, 3:28 PM

Change 495912 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Add new stateful component: imageCarousel.js

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

gerritbot added a comment.Mar 12 2019, 3:28 PM

Change 495913 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Create new helper function: makeImagesAndStartIndex

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

• nray moved this task from Doing to Needs Code Review on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q3) board.Mar 12 2019, 3:30 PM
Jdlrobson mentioned this in T211162: [Bug] Exiting and re-entering the image gallery via browser history loses page scroll offset.Mar 12 2019, 7:07 PM
gerritbot added a comment.Mar 12 2019, 7:45 PM

Change 495982 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/MinervaNeue@master] Make minerva exclusively use string emitted by image overlay onSlide event

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

gerritbot added a comment.Mar 13 2019, 7:57 PM

Change 495909 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Add new presentational component: carousel.js

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

gerritbot added a comment.Mar 14 2019, 3:41 PM

Change 496468 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Improve carousel API

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

gerritbot added a comment.Mar 14 2019, 5:56 PM

Change 496468 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Improve carousel API

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

ReleaseTaggerBot added a project: MW-1.33-notes (1.33.0-wmf.22; 2019-03-19).Mar 16 2019, 8:31 PM
gerritbot added a comment.Mar 18 2019, 3:34 PM

Change 495911 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Add new presentational component: imageDetails.js

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

gerritbot added a comment.Mar 18 2019, 10:49 PM

Change 497422 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Improve imageDetails.js API

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

• Niedzielski moved this task from Needs Code Review to Needs More Work on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q3) board.Mar 19 2019, 5:15 PM
• nray moved this task from Needs More Work to Doing on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q3) board.Mar 19 2019, 5:38 PM
gerritbot added a comment.Mar 19 2019, 9:04 PM

Change 497422 had a related patch set uploaded (by Jdlrobson; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Improve imageDetails.js API

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

gerritbot added a comment.Mar 19 2019, 9:19 PM

Change 497422 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Improve imageDetails.js API

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

ReleaseTaggerBot edited projects, added MW-1.33-notes (1.33.0-wmf.23; 2019-03-26); removed MW-1.33-notes (1.33.0-wmf.22; 2019-03-19).Mar 19 2019, 10:01 PM
gerritbot added a comment.Mar 22 2019, 12:46 AM

Change 498279 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] ImageOverlay components take 2

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

gerritbot added a comment.Mar 22 2019, 5:28 PM

Change 493630 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Refactor ImageOverlay to use new stateful and presentational components

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

gerritbot added a comment.Mar 22 2019, 5:34 PM

Change 495248 abandoned by Nray:
Make ImageGateway a required prop for image overlay factory cleanup

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

gerritbot added a comment.Mar 22 2019, 5:35 PM

Change 495982 abandoned by Nray:
Make minerva exclusively use string emitted by image overlay onSlide event

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

gerritbot added a comment.Mar 22 2019, 7:45 PM

Change 498279 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Refactor ImageOverlay into an Overlay with an ImageCarousel

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

gerritbot added a comment.Mar 22 2019, 11:02 PM

Change 493630 abandoned by Nray:
Refactor ImageOverlay to use new stateful and presentational components

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

gerritbot added a comment.Mar 22 2019, 11:11 PM

Change 495912 abandoned by Nray:
Add new stateful component: imageCarousel.js

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

gerritbot added a comment.Mar 22 2019, 11:11 PM

Change 495910 abandoned by Nray:
Add new presentational component: promisedImage

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

gerritbot added a comment.Mar 22 2019, 11:12 PM

Change 495913 abandoned by Nray:
Create new helper function: makeImagesAndStartIndex

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

Jdlrobson updated the task description. (Show Details)Mar 23 2019, 12:54 AM
Jdlrobson updated the task description. (Show Details)Mar 23 2019, 12:56 AM
Jdlrobson moved this task from Doing to Needs More Work on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q3) board.

m.define( 'mobile.mediaViewer/ImageOverlay', ImageOverlay );

I noticed this line in src/mobile.mediaViewer/mobile.mediaViewer.js but the method is deprecated. We should officially deprecate this

m.deprecate( 'mobile.mediaViewer/ImageOverlay', ImageOverlay );

and update Minerva to use the new factory method.

I think we've all had enough refactoring for the time being :) so we'll regroup in a week to work out how we want to tackle these going forward.

gerritbot added a comment.Mar 25 2019, 6:30 PM

Change 495249 abandoned by Nray:
Make skins.minerva.scripts.init.js use new image overlay factory function

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

• nray moved this task from Needs More Work to Doing on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q3) board.Mar 25 2019, 6:31 PM
gerritbot added a comment.Mar 25 2019, 8:06 PM

Change 498983 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Add mediaViewer to mobile.startup exports

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

gerritbot added a comment.Mar 25 2019, 8:19 PM

Change 498985 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/MinervaNeue@master] Make minerva use updated mediaViewerOverlay factory function

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

• nray moved this task from Doing to Needs Code Review on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q3) board.Mar 25 2019, 8:38 PM
gerritbot added a comment.Mar 25 2019, 9:30 PM

Change 498983 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Add mediaViewer to mobile.startup exports

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

gerritbot added a comment.Mar 25 2019, 9:30 PM

Change 498985 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Make minerva use updated mediaViewerOverlay factory function

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

gerritbot added a comment.Mar 25 2019, 10:09 PM

Change 498998 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Make ImageCarousel able to discard old promises

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

• nray added a comment.Mar 26 2019, 4:22 PM

One more patch that I'd really like to get out of the way before moving this do QA/Ready for sign off:

https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/498998/

• nray mentioned this in T219410: Make ImageCarousel discard old promises.Mar 27 2019, 4:01 PM
• nray moved this task from Needs Code Review to Needs QA on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q3) board.Mar 27 2019, 4:55 PM
• nray updated the task description. (Show Details)
• nray added a comment.Mar 27 2019, 4:57 PM
In T216198#5058762, @nray wrote:

One more patch that I'd really like to get out of the way before moving this do QA/Ready for sign off:

https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/498998/

Decided to split the above patch into T219410 as I don't think its a trivial change and will take some time to sort out

• nray reassigned this task from • nray to Edtadros.Mar 27 2019, 5:46 PM
• nray updated the task description. (Show Details)
• nray updated the task description. (Show Details)
• nray subscribed.
• nray updated the task description. (Show Details)Mar 27 2019, 5:48 PM
• nray updated the task description. (Show Details)Mar 27 2019, 5:52 PM
Edtadros added a comment.Mar 30 2019, 1:27 AM

Test Result

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

Test Artifact(s):

QA steps

  1. Visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Barack_Obama_T173949
  2. Click on one of the images on the page
  3. Expected: The black image overlay appears with it eventually loading an image
    T216198-1.png (2×1 px, 2 MB)
  4. When the image loads, verify that clicking the black part of the overlay (but not the navigation controls) or on the image itself toggles the visibility of the navigation controls and white details bar. Note: this is intentionally differs from production's behavior which also toggles the visibility of the overlay's close icon.
    T216198-2.png (2×1 px, 2 MB)
  5. When the image loads, test that the back and forward buttons allow you to cycle through the images in the correct order
    T216198-3.png (2×1 px, 143 KB)
  6. Disconnect your internet connection, click the forward button, and verify that a load error message appears.
    T216198-4.png (2×1 px, 81 KB)
  7. Turn connection back on, and click the "Refresh" link. Verify that an image successfully loads
    T216198-5.png (2×1 px, 1 MB)
  8. Close the overlay by clicking on the close icon. Click your browser's forward button and verify that the image you just viewed appears again.
    T216198-5.png (2×1 px, 1 MB)
Edtadros reassigned this task from Edtadros to ovasileva.Mar 30 2019, 1:29 AM
Edtadros updated the task description. (Show Details)
Edtadros moved this task from Needs QA to Ready for Signoff on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q3) board.
Edtadros subscribed.
MBinder_WMF edited projects, added Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q4); removed Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q3).Apr 1 2019, 5:25 PM
MBinder_WMF moved this task from To Do to Ready for Signoff on the Web-Team-Backlog (Readers-Web-Kanbanana-Board-2018-19-Q4) board.Apr 1 2019, 5:26 PM
Edtadros added a project: Product-QA (RW-Test-Cases).Apr 4 2019, 1:49 PM
ovasileva closed this task as Resolved.Apr 8 2019, 11:02 AM
• nray mentioned this in T220862: Remove hideOnExitClick from ImageCarousel prototype.Apr 12 2019, 11:10 PM
• nray mentioned this in T220869: Remove `hasLoadError` instance variable/logic from Overlay/OverlayManager.Apr 13 2019, 1:23 AM
• nray mentioned this in T221813: Make ImageGateway request a batch of image data for ImageCarousel's use.Apr 25 2019, 12:10 AM
ovasileva moved this task from Backlog to Advanced Mobile Contributions on the Product-QA (RW-Test-Cases) board.Mar 2 2020, 6:39 PM
Restricted Application added a subscriber: Masumrezarock100. · View Herald TranscriptMar 2 2020, 6:39 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and 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 · Credits