Page MenuHomePhabricator

PageActions Menu should use Builder pattern and reuse existing Menu elements
Closed, ResolvedPublic5 Story Points

Description

In the T214715: [Spike] How should menus work in Minerva? we did a research on Menu building in Minerva skin. We came to conclusion, that we can and we should construct all menus in the same way. This approach will

  • give us more flexibility, it should be easy to modify/replace menu elements
  • extract PageActions logic from SkinMinerva class
  • allow us to unit test menu building logic
  • reduce the way how we build menus

In the T216152: AMC Navigation - add new links to main menu with click tracking we re-organized the code and used the Builder pattern to build MainMenu. In the same time we merged T216418: AMC Navigation - overflow menu which introduces the new overflow menu.
We should refactor the PageActions menu building to match new way how menus are built.
The proof-of-concept is available on gerrit: rSMIN9909b2d05d2d: POC: PageActions menu built using Builder pattern

Acceptance Criteria
  • PageActions menu building logic is removed from SkinMinerva class
  • PageActions menu is built using Builder pattern. Menu/Group and Menu/IMenuElement are used as building blocks
  • Any extensions that inject menu elements to the Page actions bar still works properly
  • Shared logic from SkinMinerva class (if any) should live as a separate service that is used both in SkinMinerva and PageActions menu builder
  • The Page Actions menu works same way as it used to
QA Steps

On Beta cluster verify:

  • PageActions works the same as it used to for anonymous user
  • PageActions works the same for AMC user
  • PageActions works the same for non-amc user
  • PageActions overflow menus work the same for NS_MAIN and NS_USER namespaces

Additionally on local env/reconfigured staging please verify

  • Toolbar menu works as expected when the AMC mode is disabled (Important) - staging env has AMC mode disabled for the test
  • PageActions works properly if OverflowMenu is not enabled

QA Results

Event Timeline

pmiazga created this task.Apr 24 2019, 4:34 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 24 2019, 4:34 PM
pmiazga triaged this task as High priority.Apr 24 2019, 4:35 PM
pmiazga moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.

We discussed this some and think the menu system should be modified minimally. Whenever a new menu item is added, it will be placed in the action bar. Except if there are already four items items there, then it will be placed in the submenu. This will allow us to use the new menu building system without changing the existing design or spending more time thinking about how to build the perfect menu system.

pmiazga updated the task description. (Show Details)Apr 24 2019, 8:37 PM

Per async estimation: Piotr as the only one voted medium, everyone else (Jon, Nick, Jan, Stephen) voted Large.

pmiazga set the point value for this task to 5.May 1 2019, 4:19 PM
pmiazga moved this task from Needs Prioritization to Upcoming on the Readers-Web-Backlog board.

Change 510101 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/skins/MinervaNeue@master] Hygiene: Deduplicate MenuEntry uniqness check code

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

Change 510125 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/skins/MinervaNeue@master] Introduce new SimpleMenuElementWithIcon

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

Change 510101 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Hygiene: Deduplicate MenuEntry uniqness check code

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

Change 511756 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/skins/MinervaNeue@master] PageActions menu elements should be built by using IMenuElement

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

Change 512224 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/skins/MinervaNeue@master] Untangle watchstar icon

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

Change 512225 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/skins/MinervaNeue@master] Watchstar should respect iewmywatchlist|editmywatchlist permissions

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

Change 512295 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/skins/MinervaNeue@master] Use Builder pattern when

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

Change 512300 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/skins/MinervaNeue@master] Hygiene: Use PermissionsManager instead of deprecated methods

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

Change 512306 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/skins/MinervaNeue@master] Hygiene: Extract isAllowedPageAction into MinervaPermissions

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

Change 512449 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/skins/MinervaNeue@master] Hygiene: SkinUserPageHelper should support Title = null

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

Change 512225 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Watchstar should respect viewmywatchlist|editmywatchlist permissions

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

Change 512449 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Hygiene: SkinUserPageHelper should support Title = null

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

Change 512224 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Untangle watchstar icon

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

Change 511756 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] PageActions menu elements should be built by using IMenuElement

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

Change 512295 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] PageActions menu should use Builder pattern

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

Change 512300 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Hygiene: Use PermissionsManager instead of deprecated methods

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

nray added a subscriber: nray.Jun 7 2019, 10:09 PM

@pmiazga is there anything left to code review on this?

Change 510125 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Introduce new SingleMenuEntry

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

pmiazga claimed this task.Jun 19 2019, 5:03 PM

Change 518288 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/skins/MinervaNeue@master] MinervaPermissions must respect $wgHideInterlanguageLinks config

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

Change 512306 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Hygiene: Extract isAllowedPageAction into MinervaPagePermissions

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

Change 518288 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] MinervaPermissions must respect $wgHideInterlanguageLinks config

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

pmiazga reassigned this task from pmiazga to Edtadros.Jun 24 2019, 9:08 PM
pmiazga updated the task description. (Show Details)
pmiazga updated the task description. (Show Details)Jun 26 2019, 5:53 PM
Edtadros added a comment.EditedJun 27 2019, 2:22 PM

Test Result

Status: ✅ PASS
OS: macOS Mojave
Browser: Chrome
Device: MBP
Emulated Device: iPhoneX

Test Artifact(s):

QA STeps

✅ AC1: PageActions works the same as it used to for anonymous user
The language button is disabled in the screenshot below because the page wasn't available in another language.

betaeswiki

The screenshot below shows a page in beta where the language button is enabled because the page is available in another language.

✅ AC2: PageActions works the same for AMC user
The language button is disabled on this page in Beta, see AC1.

betaeswiki

✅ AC3: PageActions works the same for non-amc user
The language button is disabled on this page in Beta, see AC1.

betaeswiki

✅ AC4: PageActions overflow menus work the same for NS_MAIN and NS_USER namespaces
The language button is disabled on this page in Beta, see AC1.

betaeswiki

Additionally on local env/reconfigured staging please verify

✅ AC5: Toolbar menu works as expected when the AMC mode is disabled (Important) - staging env has AMC mode disabled for the test
The language button is disabled in staging for a page without additional languages. See below for an example of a page with additional languages.

stagingenwiki

✅ AC6: PageActions works properly if OverflowMenu is not enabled

staging

@pmiazga thanks for your help!

@pmiazga Let me know when I can test AC6 (T221792#5289083)

Edtadros reassigned this task from Edtadros to ovasileva.Jul 3 2019, 6:12 PM
Edtadros updated the task description. (Show Details)
Edtadros added a subscriber: Edtadros.
ovasileva reassigned this task from ovasileva to Jdrewniak.Jul 8 2019, 5:06 PM
ovasileva added a subscriber: ovasileva.
Jdrewniak closed this task as Resolved.Jul 15 2019, 5:02 PM