Page MenuHomePhabricator

Menus: Make Tabs template fully-data driven to support upcoming navigation restructure work
Closed, ResolvedPublic3 Estimated Story Points

Description

In SHDT the web team discussed how in many places we are not template-driven, instead resorting to PHP functions to generate blobs of HTML. This causes a lot of slowdown in anything menu related which is becoming more of a problem as we enter Navigation Restructure (Web)

TODO

  • This task is done when the Tabs.mustache template renders individual list items via templates rather than html-items.
  • You will need to update the components inside MediaWiki core to provide access to the bare minimum data needed to support the new template.
  • The scope for this change is Tabs.mustache (the talk and main namespace). To limit scope you will likely need to fork the MenuContents template / associated component. Try not to impact any other menu in your solution.
  • This change can add new keys to the existing data, but should preserve any existing keys and their existing types for backwards compatibility e.g. do not remove any keys e.g. html-items.

Existing code

<ul class="vector-menu-content-list">{{!
        }}{{#data-items}}{{>MenuListItem}}{{/data-items}}{{!
        }}{{{html-items}}}</ul>

Final code

<ul class="vector-menu-content-list">{{!
        }}{{#data-items}}{{>MenuListItem}}{{/data-items}}{{#array-items}}{{>ListItem}}{{/array-items}}</ul>

Event Timeline

I think the code examples in the description could probably be updated, {{#data-items}}{{>MenuListItem}}{{/data-items}} is already there for rendering MenuItems, but its not actually used for anything. I would suggest to just update that code including MenuListItem.mustache and IconLink.mustache files, rather than making new templates ListItem

We've had two icon related issues in page tools so far and we have no generic way to address these in future and menus/icons will be of extreme importance in the navigation structure project. There is considerable work to do to make this possible and this task was carved out as a small step in that direction. It's of strategic important we do this sooner rather than later if we want to eventually solve the issue of the Atom link and the interlanguage link in the page tools (T328069, T327717 and other related issues). I think it's high priority that we build on the momentum we've already created to fix this now. It's going to be harder to fix this later if we ignore this problem.

Related Patches:

TODO:

  • Fix cases as mentioned in the task TODO ... can be done as a followup
  • Cleanup and refactoring of code

TODO:

  • Cleanup and refactoring of code
  • Found a number of pixel regressions, need to pick a solution:
    • Wrap Tabs changes in a new template just for tabs, until we do roll other links/list items. Create a followup for regression prevention on other menu types.
    • Increase the scope of this task and the estimate points and spend the time on fixing it

@Jdlrobson
@olga
@NHillard-WMF

Please advice on the preferred course of actions. I will do the 1st TODO asap, then will check the board if I can pick something else ...

@Mabualruz I'm not sure I understand your question and what this is blocked on? We shouldn't increase the scope of this task or re-estimate.

Using a new template sounds fine (and is mentioned in the acceptance criteria, specifically: "To limit scope you will likely need to fork the MenuContents template / associated component. Try not to impact any other menu in your solution.: It should be possible to swap the tab templates without triggering Visual regressions. What regressions are you seeing?

@Mabualruz I'm not sure I understand your question and what this is blocked on? We shouldn't increase the scope of this task or re-estimate.

Using a new template sounds fine (and is mentioned in the acceptance criteria, specifically: "To limit scope you will likely need to fork the MenuContents template / associated component. Try not to impact any other menu in your solution.: It should be possible to swap the tab templates without triggering Visual regressions. What regressions are you seeing?

Yea I already had a patch with Tabs Only I just need to fork the templates, will do that and check notes on patches, thanks

@Mabualruz I'm not sure I understand your question and what this is blocked on? We shouldn't increase the scope of this task or re-estimate.

Using a new template sounds fine (and is mentioned in the acceptance criteria, specifically: "To limit scope you will likely need to fork the MenuContents template / associated component. Try not to impact any other menu in your solution.: It should be possible to swap the tab templates without triggering Visual regressions. What regressions are you seeing?

@Jdlrobson The MenuContents and MenuListItem templates are both unused ATM, does it still make sense to fork those templates?
https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/893438/12/includes/templates/TabsMenuContents.mustache#9

MenuListItem is currently used inside MenuContents.mustache. MenuContents is used in multiple places - LegacyMenu, Tabs, VariantsDropdown, LanguageDropdown, MainMenuGroup, Menu, LegacyMoreMenu.
For this task we are intentionally scoping to Tabs template, so we should only be modifying includes/templates/Tabs.mustache (not MainMenuGroup) to use a new forked version of MenuContents. Does this make sense?

Change 893438 had a related patch set uploaded (by Jdlrobson; author: Mabualruz):

[mediawiki/skins/Vector@master] [vector] Menus: Make Tabs template fully-data driven to support upcoming navigation restructure work

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

Change 893436 had a related patch set uploaded (by Jdlrobson; author: Mabualruz):

[mediawiki/core@master] [core] Menus: Make Tabs template fully-data driven to support upcoming navigation restructure work

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

MenuListItem is currently used inside MenuContents.mustache. MenuContents is used in multiple places - LegacyMenu, Tabs, VariantsDropdown, LanguageDropdown, MainMenuGroup, Menu, LegacyMoreMenu.
For this task we are intentionally scoping to Tabs template, so we should only be modifying includes/templates/Tabs.mustache (not MainMenuGroup) to use a new forked version of MenuContents. Does this make sense?

MenuContents.mustache uses MenuListItem when 'data-items' is provided, but none of our menus use 'data-items' so MenuListItem is effectively unused. All of our menus use still use 'html-items' instead. Regardless I'm fine with this patch as long as TabMenuContens and TabMenuListItem are cleaned up soon or captured in another task

Okay I see. Mm it looks like we were using this in https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/876041/12/includes/SkinVector22.php#208 but at some point usage was removed. Whenever it was removed, the template code should also have been moved, so perhaps these should be removed in a separate patchset so that's not confusing.

Change 895742 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/skins/Vector@master] [core] Menus: Make Tabs template fully-data driven to support upcoming navigation restructure work

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

Change 893436 had a related patch set uploaded (by Jdlrobson; author: Mabualruz):

[mediawiki/core@master] [core] Menus: Make Tabs template fully-data driven to support upcoming navigation restructure work

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

@bwang https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/896147 removes that template. Presuming Pixel is fine I think it's okay to merge as part of this ticket.

Change 896147 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] Drop unused template and associated code

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

Change 896147 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Drop unused template and associated code

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

Change 895742 had a related patch set uploaded (by Jdlrobson; author: Mabualruz):

[mediawiki/skins/Vector@master] [vector] Menus: Make Tabs template fully-data driven to support upcoming navigation restructure work

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

Change 893436 had a related patch set uploaded (by Jdlrobson; author: Mabualruz):

[mediawiki/core@master] [core] Menus: Make Tabs template fully-data driven to support upcoming navigation restructure work

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

Change 895742 had a related patch set uploaded (by Jdlrobson; author: Mabualruz):

[mediawiki/skins/Vector@master] [vector] Menus: Make Tabs template fully-data driven to support upcoming navigation restructure work

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

Change 893436 had a related patch set uploaded (by Mabualruz; author: Mabualruz):

[mediawiki/core@master] [core] Menus: Make Tabs template fully-data driven to support upcoming navigation restructure work

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

Change 893436 merged by jenkins-bot:

[mediawiki/core@master] [core] Menus: Make Tabs template fully-data driven to support upcoming navigation restructure work

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

Change 895742 merged by jenkins-bot:

[mediawiki/skins/Vector@master] [vector] Menus: Make Tabs template fully-data driven to support upcoming navigation restructure work

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

This can skip QA as this is covered by automated tests e.g. Pixel

This has the Pixel stamp of approval.