Page MenuHomePhabricator

MFA: LanguageOverlay should be an Overlay with a LanguageSearcher component
Closed, ResolvedPublic8 Story Points

Description

The LanguageOverlay is an Overlay which overrides templatePartials.content to replace the body of the overlay element.
Code: https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/src/mobile.languages.structured/LanguageOverlay.js

Similar to how we refactor the TalkOverlay in T215370 we will convert this to an Overlay which has a LanguageSearcher child.

This new component will look like this:

This change will span 2 directories.

Acceptance criteria

  • Minerva uses a languageOverlay factory function for generating the overlay used for the language feature inside resources/skins.minerva.scripts/init.js. As before it is associated with the route #/language
  • There is no LanguageOverlay
  • A new language widget is created
  • The new language searcher component can be rendered outside an Overlay if wanted.
  • Styles referring to the language-overlay class are scoped to the new component. Feel free to make use of BEM if that makes sense with the new class names.
  • No changes have been made to the underlying behaviour of the overlay
  • Consider moving the async api call from Minerva to getLanguages into the newly created MobileFrontend factory function (https://github.com/wikimedia/mediawiki-skins-MinervaNeue/blob/master/resources/skins.minerva.scripts/init.js#L158) -- this seems to be in languageOverlay#loadLanguageSearcher()

QA steps

Exploratory testing needed for visual regressions and bugs.

Visit https://en.m.wikipedia.beta.wmflabs.org/w/index.php?title=Language_test&mobileaction=toggle_view_mobile

  • Click the language button to open the language overlay. This is what we want to test.
  • Test the search filter feature by typing into "Search for a language"
  • Test closing via back button and close icon
  • Check you can click languages
  • Close overlay and open again
  • When clicking a language, we store that as a preferred language. Check that any previously clicked languages show on subsequent visits in SUGGESTED LANGUAGES

When looking for UI regressions compare all parts of the language workflow with what's currently on production, accessible via:
https://en.wikipedia.org/wiki/Barack%20Obama#/languages

Note there is one expected UI regression: the spinner will now show inside the body of the language overlay

Sign off steps

  • The newly created LanguageSearcher will have less than the desired amount of code coverage. It may be appropriate as part of this task to bump code coverage to 100% but it may be better to spin this out into a new task.-- Both languageOverlay and LanguageSearcher.js have 100% coverage.

QA Results

StatusDetails))
✅ PassedT215657#4983613

Event Timeline

Jdlrobson triaged this task as High priority.Feb 8 2019, 8:58 PM
Jdlrobson created this task.
Jdlrobson moved this task from Incoming to Upcoming on the Readers-Web-Backlog board.

Change 490635 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] WIP Convert LanguageOverlay into a overlay factory composed of a LanguageSearcher

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

phuedx removed a subscriber: phuedx.Feb 14 2019, 4:39 PM

Change 490637 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/MinervaNeue@master] WIP Call new language factory instead of LanguageOverlay

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

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

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

Change 491512 abandoned by Nray:
Add AsyncContentOverlay component

Reason:
Abandoned in favor of styling the spinner container that PromisedView sets up and applying some base styles to overlay-content as shown with Overlay.less in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/ /490635/10/resources/mobile.startup/Overlay.less

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

Change 491829 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Apply view--loading class to PromisedView's parent element when loading

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

Change 491881 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Inline LanguageSearcher template

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

Change 491829 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Apply promised-view class to PromisedView's parent element when loading

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

Change 490635 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Make LanguageOverlay an overlay composed of a LanguageSearcher component

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

Change 490637 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Call new language factory instead of LanguageOverlay

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

Jdlrobson reassigned this task from nray to Edtadros.Feb 21 2019, 10:25 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson added subscribers: Edtadros, nray.

@nray I think QAing this now is important so added @Edtadros so we can start that now. This will be on the train Monday, so let's try and catch any major issues before then.

Please continue to submit patches and we'll coordinate extra testing as necessary with Edward. :)

Change 492204 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Fix @memberof in LanguageSearcher

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

Change 492205 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Update reference to languageOverlay in Overlay.less

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

Change 492206 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Remove temporary function from mobile.languages.structured.js

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

Change 492207 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/MinervaNeue@master] Move/rename LanguageOverlay.less to mobile.startup/languageOverlay.less

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

Change 492204 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Fix @memberof in LanguageSearcher

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

Change 492205 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Update reference to languageOverlay in Overlay.less

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

Change 492359 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/MinervaNeue@master] Import all skinStyles/mobile.startup into mobile.startup.less

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

Change 492207 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Refactor LanguageOverlay styles

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

Change 492359 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Import all skinStyles/mobile.startup into mobile.startup.less

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

Change 492383 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Remove usages of overlay.$() from languageOverlay.test.js

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

Change 492383 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove usages of overlay.$() from languageOverlay.test.js

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

Jdlrobson updated the task description. (Show Details)Feb 25 2019, 11:58 PM

Test Result

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

Test Steps and Artifact(s):

StepDescriptioinStatusArtifact
1Visit https://en.m.wikipedia.beta.wmflabs.org/w/index.php?title=Language_test&mobileaction=toggle_view_mobile
2Click the language button to open the language overlay. This is what we want to test.
3Test the search filter feature by typing into "Search for a language"
4Test closing via back button and close icon
5Check you can click languages
6Close overlay and open again
7When clicking a language, we store that as a preferred language. Check that any previously clicked languages show on subsequent visits in SUGGESTED LANGUAGES
Jdlrobson added a subscriber: ovasileva.

@nray should we cut a new task for increasing code coverage of the LanguageSearcher component or do you want to do this as part of this work?

nray added a comment.Feb 26 2019, 6:43 PM

@Jdlrobson I'm happy to do it as part of this task

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 492940 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Move promisedView styles out of languageOverlay styles

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

Change 493272 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Increase LanguageSearcher code coverage to 100%

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

nray added a comment.Feb 27 2019, 4:50 PM

@Jdlrobson language searcher code coverage patch is at https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/493272/

Should I increase coverage for src/mobile.languages.structured/util.js as well? That one also has less than 100

Change 493272 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Increase LanguageSearcher code coverage to 100%

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

Niedzielski closed this task as Resolved.Feb 27 2019, 8:57 PM
Niedzielski updated the task description. (Show Details)