Page MenuHomePhabricator

TalkOverlay should not extend Overlay
Closed, ResolvedPublic5 Story Points

Description

The TalkOverlay inside mobile.talk.overlays is one of our most simple overlays.

It's an overlay with

  • a header
    • that contains an "add discussion" button
  • a content area
  • that contains a Panel component "The following conversations are currently active"
  • that contains a list of topics.
  • a footer
  • which contains a single link.

It looks like this:

Despite this, the TalkOverlay is defined as a single component. It uses templatePartials from other classes and using a gateway makes ajax requests!

The same overlay can be created via composition using an async function

overlayManager.addRoute( '/talk/', () => {
    return gateway.getTopics( url ).then( ( topics ) => {
    new Overlay( {
    events: { .... },
    action: new Button( { label: 'Add discussion' } ),
        anchor: new Anchor( { label: 'read as wiki page' } ),
        children: [ new Panel(), new TopicTitleList( topics ) ]
    } );
    } );
}

Developer notes

I took a stab at this here: https://gerrit.wikimedia.org/r/471334

I found one component particularly useful - The PromisedView component. This is essentially a temporary view that replaces itself when a promise is resolved (essentially an async component).

I also saw an opportunity to tidy up some CSS/HTML generation shared by category, talk and language overlay.

Please see the pageIssuesOverlay in Minerva for an existing example of an Overlay not using composition.

Note: TalkSectionOverlay and TalkSectionAddOverlay are out of scope for this task.

Acceptance criteria

  • The Overlay class is updated to be flexible enough to render the TalkOverlay
  • The TalkOverlay has no template partials (it instead uses child components)
  • The TalkOverlay is actually an Overlay
  • We have a way of creating async components that depend on data from API calls.
  • We have a working agreed upon way of doing composition given the constraints of the system
  • A conversation has been had about how this went and how we might do it better next time.

Descoped acceptance criteria

  • A reusable component (suggested name TopicTitleList) is created that can be used inside the CategoryOverlay and LanguageOverlay:



T173534

QA steps

  • Test this on the beta cluster: https://en.m.wikipedia.beta.wmflabs.org/wiki/Spain
  • Visit article in stable mode while logged in
  • Click the talk button (bottom or top of article)
  • Verify several talk topics show
  • Verify that when you click one of the topics you are shown that topic
  • Verify that the talk overlay can be closed by the browser back button
  • Verify that the talk overlay can be closed by clicking the icon in the top left
  • Verify that clicking add discussion shows a form for adding new topics

Sign off steps

  • Set up tasks for TalkSectionOverlay and TalkSectionAddOverlay which we will port next (see T217102)
  • Setup a task for moving the registration for the talkOverlay route into MobileFrontend and removing the usage of LoadingOverlay / loadModule. (This was done as part of this change!)

QA Results

StatusDetails))
✅ PassedT215370#4983605

Event Timeline

Jdlrobson triaged this task as High priority.Feb 6 2019, 1:09 AM
Jdlrobson created this task.
Jdlrobson moved this task from To Triage to Upcoming on the Readers-Web-Backlog board.
phuedx added a comment.Feb 6 2019, 9:56 AM

AFAICT there's no instrumentation about how (or how frequently) our various overlays are being interacted with. Nevertheless, it's a production feature, so we should do our best to make sure that we don't introduce regressions while doing this refactoring. With that in mind, could we add tests so that this can go through Needs QA.

Jdlrobson updated the task description. (Show Details)Feb 6 2019, 4:09 PM
Jdlrobson updated the task description. (Show Details)Feb 6 2019, 4:36 PM
ovasileva set the point value for this task to 5.Feb 12 2019, 5:40 PM

We discussed in grooming today that having a promisedView component would have some significant benefits and make this work easier, so we should create a task to document that work and consider that a dependency for this task.

We discussed in grooming today that having a promisedView component would have some significant benefits and make this work easier, so we should create a task to document that work and consider that a dependency for this task.

Done in T215972!

Change 471334 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] A simplified TalkOverlay

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

Change 492211 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Move talkOverlay factory to the mobile.startup module

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

Change 492212 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Talk overlay no longer uses the loadingOverlay pattern

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

Change 471334 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] A simplified TalkOverlay

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

Change 492211 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Move talkOverlay factory to the mobile.startup module

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

Change 492212 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Talk overlay no longer uses the loadingOverlay pattern

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

Jdlrobson reassigned this task from Jdlrobson to Edtadros.Feb 25 2019, 7:57 PM

While Edward QAs I will work on some minor follow ups :

  1. the spinner is not centered when loading ( and combined with the minerva patch). Looks like this is the case with master too though so it doesn't have to be a blocker.
  2. Explore using jQuery.when inside src/mobile.startup/talk/overlay.js for 2 parallel requests
Edtadros reassigned this task from Edtadros to ovasileva.Feb 26 2019, 5:52 AM
Edtadros added a subscriber: Edtadros.

=== Test Result

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

Test Steps and Artifact(s):

StepDescriptionStatusArtifact
1Test this on the beta cluster: https://en.m.wikipedia.beta.wmflabs.org/wiki/Spain
2Visit article in stable mode while logged in
3Click the talk button (bottom or top of article)
4Verify several talk topics show
5Verify that when you click one of the topics you are shown that topic
6Verify that the talk overlay can be closed by the browser back button
7Verify that the talk overlay can be closed by clicking the icon in the top left
8Verify that clicking add discussion shows a form for adding new topics
Edtadros updated the task description. (Show Details)Feb 26 2019, 5:53 AM
Jdlrobson reassigned this task from ovasileva to pmiazga.Feb 26 2019, 6:08 PM
Jdlrobson added a subscriber: ovasileva.

Change 492939 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Provide a jquery alternative for Promise.all

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

Jdlrobson raised the priority of this task from High to Needs Triage.Feb 26 2019, 7:09 PM

Change 492940 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Move promisedView styles out of languageOverlay styles

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

Change 492939 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Provide a jquery alternative for Promise.all

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

phuedx removed a subscriber: phuedx.Feb 27 2019, 12:00 PM

Change 492940 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Move promisedView styles out of languageOverlay styles

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

Jdlrobson updated the task description. (Show Details)Feb 28 2019, 7:12 PM
ovasileva triaged this task as High priority.Mar 4 2019, 6:04 PM
pmiazga closed this task as Resolved.Mar 4 2019, 6:05 PM
pmiazga removed pmiazga as the assignee of this task.
pmiazga updated the task description. (Show Details)
pmiazga added a subscriber: pmiazga.