Page MenuHomePhabricator

PageActions Menu should use Builder pattern and reuse existing Menu elements
Open, HighPublic5 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/MenuElement 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
  • Builder should return two groups (MenuEntries stored in the bar area and menu entries stored in the overflow modal)
  • The Page Actions menu works same way as it used to

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 To Triage to Needs Analysis 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 Analysis 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.Fri, Jun 7, 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