Page MenuHomePhabricator

Simplify MainMenu building in Minerva skin
Closed, ResolvedPublic5 Estimated Story Points

Description

When working on rSMINd94103f62fef: Provide a code structure for menus handling we noticed that the Main Menu building is achieved by calling different functions. Each function is responsible for building a single menu element. That approach has led to the situation where we have many functions in the following structure:

		$group->insert( 'settings' )
			->addComponent(
				$this->context->msg( MENU_LABEL )->escaped(),
				SpecialPage::getTitleFor( SPECIAL_PAGE )->getLocalURL(),
				MinervaUI::iconClass( ICON_NAME , 'before' ),
				[ 'data-event-name' => EVENT_NAME ]
			);

To remove duplicated code, and significantly reduce the Definitions.php size, please provide a generic buildMenuEntry() function, and use this function in concrete Menu Builder classes.
To make code even more readable, buildMenuEntry() should not add menuEntry to the $group object - it should return it instead, and then the concrete Menu Builder implementation should build Menu groups.

Event Timeline

@pmiazga any updates on this task? It's in tracking - do we plan to work on this? Shall we decline/update the task description?

I have a question. It seems there are two ways to insert a menu item: with insert and insertEntry. These functions add from different classes SingleMenuEntry and MenuEntry, which are very similar. Is there a reason why some items are inserted with insertEntry? Would it be part of the scope to unify these classes?

Change 807177 had a related patch set uploaded (by Scardenasmolinar; author: Scardenasmolinar):

[mediawiki/skins/MinervaNeue@master] [WIP] Do not merge - Refactor MainMenu building

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

Hi @Scardenasmolinar I've asked @cjming to take a look at this. In quarter 1 (starting July), the web team is planning to standardize menu code in core which would replace this code. I'm sure Clare would be interested in hearing about pain points you are experiencing to inform that work.

Thanks, @Jdlrobson! I am removing some functions that appear to be unused in the code and removing one of the MenuEntry classes so that there is only one class to rule them all :)

Test wiki created on Patch demo by SCardenas (WMF) using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/4d049e340e/w/

Hi @Scardenasmolinar I've asked @cjming to take a look at this. In quarter 1 (starting July), the web team is planning to standardize menu code in core which would replace this code. I'm sure Clare would be interested in hearing about pain points you are experiencing to inform that work.

Thanks for the context - we'll move ahead with this work since it will help us understand and improve the codebase we're likely to be touching in the meantime and make sure to document pain points.

I'm sure Clare would be interested in hearing about pain points you are experiencing to inform that work.

hi @Scardenasmolinar - I definitely am! We started a goal task for building the new menu API in core T311647 -- please feel free to add any thoughts/suggestions there.

Change 807177 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Refactor MainMenu building

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

Thanks, @cjming! I have started writing down some notes on my experience simplifying the Menu and will add them to the ticket next week. I'll be creating a couple of new tickets to hard deprecate the Group::insertAfter() function in Minerva based on your review of the Gerrit patch (807177).

Test wiki on Patch demo by SCardenas (WMF) using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/4d049e340e/w/