Page MenuHomePhabricator

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

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

header

footer

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

Related Gerrit Patches:
mediawiki/skins/MinervaNeue : masterMake minerva use updated mediaViewerOverlay factory function
mediawiki/extensions/MobileFrontend : masterAdd mediaViewer to mobile.startup exports
mediawiki/skins/MinervaNeue : masterMake skins.minerva.scripts.init.js use new image overlay factory function
mediawiki/extensions/MobileFrontend : masterCreate new helper function: makeImagesAndStartIndex
mediawiki/extensions/MobileFrontend : masterRefactor ImageOverlay to use new stateful and presentational components
mediawiki/extensions/MobileFrontend : masterAdd new presentational component: promisedImage
mediawiki/extensions/MobileFrontend : masterAdd new stateful component: imageCarousel.js
mediawiki/extensions/MobileFrontend : masterRefactor ImageOverlay into an Overlay with an ImageCarousel
mediawiki/skins/MinervaNeue : masterMake minerva exclusively use string emitted by image overlay onSlide event
mediawiki/extensions/MobileFrontend : masterMake ImageGateway a required prop for image overlay factory + cleanup
mediawiki/extensions/MobileFrontend : masterImprove imageDetails.js API
mediawiki/extensions/MobileFrontend : masterAdd new presentational component: imageDetails.js
mediawiki/extensions/MobileFrontend : masterImprove carousel API
mediawiki/extensions/MobileFrontend : masterAdd new presentational component: carousel.js
mediawiki/extensions/MobileFrontend : masterRemove Overlay.prototype.hasFixedHeader
mediawiki/extensions/MobileFrontend : masterLet promisedView handle rejects gracefully
mediawiki/extensions/MobileFrontend : masterDrop ImageOverlays reliance on template partials

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ovasileva set the point value for this task to 5.Feb 19 2019, 5:47 PM

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

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!)

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

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

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

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 triaged this task as High priority.Feb 27 2019, 6:10 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

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

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

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

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

phuedx removed a subscriber: phuedx.Mar 7 2019, 11:31 AM

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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.

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

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

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

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

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

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

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

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

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 added a comment.Mar 27 2019, 4:57 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/

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 added a subscriber: nray.
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

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
  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.
  5. When the image loads, test that the back and forward buttons allow you to cycle through the images in the correct order
  6. Disconnect your internet connection, click the forward button, and verify that a load error message appears.
  7. Turn connection back on, and click the "Refresh" link. Verify that an image successfully loads
  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.
Edtadros reassigned this task from Edtadros to ovasileva.Mar 30 2019, 1:29 AM
Edtadros updated the task description. (Show Details)
Edtadros added a subscriber: Edtadros.
ovasileva closed this task as Resolved.Apr 8 2019, 11:02 AM