Page MenuHomePhabricator

Divide VectorTemplate into components
Open, MediumPublic

Description

VectorTemplate is written in a very imperative style. It reads top to bottom and inlines each detail needed, rendering as it goes. This approach may appear simple but is actually very rigid and makes it challenging and error prone to iterate on without knowing the whole thoroughly. Specifically:

  • Skin hooks are mixed with HTML. In some cases, a transition plan may be needed to change this API lifecycle but there are probably some good refactors to be made that can minimize where hooks and HTML intermingle to better separate these concerns.
  • Code is not modular. Large portions of VectorTemplate are written in lengthy functions that cannot be reused. These could be split up to enable composition, improve the flexibility for future work, and potentially improve readability (for instance, the block of code that is specific to the sidebar could be isolated to make that clear). Refactoring code into distinct functions and separate files (either Mustache templates or PHP) could be an improvement.
  • Most of the code directly renders. PHP echos and inline HTML inhibit the ability to aggregate and compose components independent of their function call order. The rendering (calling echo) should be deferred.

Acceptance criteria

  • VectorTemplate is split into distinct components: large functions are split and files are separated where it makes sense. When considering what to refactor, keep in mind that many of these components will need to be iterated on for new DIP changes while at the same supporting the old layout. For instance, it may make sense later on to have an "old" component and a "new" component if they diverge greatly.
  • Where public API changes are not needed, hook invocations are separated from the components.
  • Rendering (echo and inline HTML) is deferred.

QA

TODO: for each patch merged, update this section. It would be ideal if Edward could focus on testing an individual component instead of the entire skin. JavaScript and non-JavaScript tests should probably be performed to minimize risk.

Related prior work: https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/Vector/+/552123/

Details

Related Gerrit Patches:

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptTue, Nov 26, 5:02 PM
Niedzielski updated the task description. (Show Details)Tue, Nov 26, 5:20 PM
Niedzielski updated the task description. (Show Details)Tue, Nov 26, 5:34 PM

Change 554284 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/skins/Vector@master] Move namespace & view tabs into a VectorTabs.mustache component

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

Change 554284 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Move namespace & view tabs into a VectorTabs.mustache component

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

ovasileva triaged this task as Medium priority.Wed, Dec 4, 5:27 PM

Change 555626 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/Vector@master] Fix: tab attributes

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

Change 555628 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/Vector@master] [Hygiene] [Mustache] improve template parameters and docs

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

Change 555630 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/Vector@master] [Hygiene] [Mustache] rename SearchComponent to SearchBox

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

Change 555626 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Fix: tab attributes

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

Change 555628 merged by jenkins-bot:
[mediawiki/skins/Vector@master] [Hygiene] [Mustache] improve template parameters and docs

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

Change 555630 merged by jenkins-bot:
[mediawiki/skins/Vector@master] [Hygiene] [Mustache] rename SearchComponent to SearchBox

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