Page MenuHomePhabricator

[EPIC] Proposal: abandon use of template partials in MobileFrontend for components within components e.g. Button, Icon and Anchor classes
Closed, ResolvedPublic

Description

[this is a placeholder and prone to change - please ask questions in comments]

The MobileFrontend View class encourages the overriding of a template partials by exposing a templatePartials attribute. Now the extension is mature, this seems to actually make things more difficult. Rather than having generic reusable templates we end up with many near identical templates. This is most notably observed in the classes which extend Overlays - overlays often have to replace entire template partials for headers which contain very similar HTML. It also encourages inconsistent mark up and class names.

Another sign this is a bad approach is the inconsistent use of toHtmlString and iconOptions / buttonOptions. On many occasions, specifically with Button and Icon classes - we want to include them inside a template partial. This is usually done by one of these two methods - the template for Button is used as a partial or we render a class with a template and then extract the outputted HTML. As Timo pointed out in T129943 this is very wasteful and potentially a performance problem (full text below).

If you look at OOjs UI it handles this by appending elements where needed. We could replicate such an approach by using the following kind of pattern:

var btn = new Button();
this.$( '.btn-holder' ).append( btn.$el );

We could also make many of our templates - in particular overlay headers more generic so that it is easier to create new overlays (see T139798). This work would benefit the work Joaquin and Sam have been doing with turning this code into a reusable library mfui.

Proposal

  • Migrate to composition approach rather than combining template partials
  • Generalise all main template files so inheriting classes do not need to override them
  • Remove the templatePartials property from View class
Original report by @Krinkle at T129943:

Disclaimer: I've only run through the code once and may've misunderstood parts.
Typical usage:

// Via icons factory
icons.spinner().toHtmlString();
// Or directly
new Icon( .. ).toHtmlString();
  • Constructor (View#initialize)
    • The Icon class in MobileFrontend JavaScript extends the mobile.startup/View class.
    • Given that Icon has a Hogan template set, View#initialize takes the if template path where possible and precompiles the template for later use.
    • The Icon class still has a tagName configured (not sure why, since it uses a template) and View creates an empty DOM element to be never used.
    • Further down the constructor it calls View#render(). I'd expect a consumer to have to all render() or indirectly by another method, not in the constructor already.
  • Render (View#render)
    • Icon has a template and isTemplateMode, so render():
    • Calls Hogan to produce an HTML string.
    • Discards the previously created DOM element.
    • Implicitly parses the HTML into a new DOM element with jQuery.
  • Embed (Icon#toHtmlString)
    • Creates another DOM element.
    • Appends the rendered node.
    • Calls on jQuery to let the browser serialise the node back to an HTML serialised string.
    • Embed the string in a larger HTML template, or call on jQuery to let the browser parse the HTML back into (another) DOM node. This all seems quite wasteful.

Sign off steps

  • A grep for >anchor returns 0 results in MobileFrontend and Minerva
  • A grep for >button returns 0 results in MobileFrontend and Minerva
  • A grep for >icon returns 0 results in MobileFrontend and Minerva
  • A github search for code using "Icon.prototype.template" shows no results
  • A github search for code using "Anchor.prototype.template" shows no results
  • [] A github search for code using "Button.prototype.template" shows no results

Details

Related Gerrit Patches:
mediawiki/extensions/MobileFrontend : masterDrop Anchor templatePartial usage in Overlays and CtaDrawer
mediawiki/extensions/MobileFrontend : masterDrop Icon and Button partial usages outside those classes
mediawiki/extensions/MobileFrontend : masterSearchOverlay doesn't make use of Icon template partial
mediawiki/extensions/MobileFrontend : masterEditorOverlay anonymous warning buttons are appended
mediawiki/extensions/MobileFrontend : masterDrop ImageOverlays reliance on template partials
mediawiki/extensions/MobileFrontend : masterTalk overlays should not need partials for headers

Related Objects

StatusAssignedTask
OpenNone
OpenNone
OpenNone
Resolved Jdlrobson
OpenNone
Resolved Jdlrobson
Resolvedpmiazga
Resolved Jdlrobson
DeclinedNone
Resolved Jdlrobson
Resolved Jdlrobson
DeclinedNone
ResolvedNiedzielski
DuplicateReedy
ResolvedNiedzielski
Resolved Jdlrobson
Resolved Jdlrobson
DuplicateNone
Resolvedpmiazga
Resolvedovasileva
Resolvedovasileva
Resolvedovasileva

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 3 2016, 4:02 PM
Krinkle removed a subscriber: Krinkle.

Change 315428 had a related patch set uploaded (by Jdlrobson):
Talk overlays should not need partials for headers

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

Change 315428 abandoned by Jdlrobson:
Talk overlays should not need partials for headers

Reason:
To be continued...

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

Krinkle removed a subscriber: Krinkle.Jan 8 2018, 6:19 PM

From https://github.com/developit/vhtml

New: "Sortof" Components!
vhtml intentionally does not transform JSX to a Virtual DOM, instead serializing it directly to HTML. However, it's still possible to make use of basic Pure Functional Components as a sort of "template partial".
When vhtml is given a Function as the JSX tag name, it will invoke that function and pass it { children, ...props }. This is the same signature as a Pure Functional Component in react/preact, except children is an Array of already-serialized HTML strings.
This actually means it's possible to build compositional template modifiers with these simple Components, or even higher-order components.
Here's a more complex version of the previous example that uses a component to encapsulate iteration items:

This sounds really interesting, and would be useful as an intermediate step towards using Preact...

Krinkle updated the task description. (Show Details)Dec 3 2018, 3:16 AM
Krinkle added a subscriber: Krinkle.

Change 482252 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Drop Anchor templatePartial usage in Overlays and CtaDrawer

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

Jdlrobson renamed this task from [EPIC] Proposal: abandon use of template partials in MobileFrontend to [EPIC] Proposal: abandon use of template partials in MobileFrontend for Button, Icon and Anchor classes.Jan 4 2019, 6:18 PM
Jdlrobson renamed this task from [EPIC] Proposal: abandon use of template partials in MobileFrontend for Button, Icon and Anchor classes to [EPIC] Proposal: abandon use of template partials in MobileFrontend for components within components e.g. Button, Icon and Anchor classes .

Change 482382 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Drop ImageOverlays reliance on template partials

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

Change 482383 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] EditorOverlay anonymous warning buttons are appended

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

Change 482384 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] SearchOverlay doesn't make use of Icon template partial

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

Change 482385 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Drop Icon and Button partial usages outside those classes

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

Change 482382 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Drop ImageOverlays reliance on template partials

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

Change 482383 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] EditorOverlay anonymous warning buttons are appended

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

Krinkle removed a subscriber: Krinkle.Apr 17 2019, 6:27 PM

Change 482384 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] SearchOverlay doesn't make use of Icon template partial

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

Change 482385 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Drop Icon and Button partial usages outside those classes

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

Change 482384 abandoned by Jdlrobson:
SearchOverlay doesn't make use of Icon template partial

Reason:
Merged into https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/ /482385/

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

Change 482385 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Drop Icon and Button partial usages outside those classes

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

Change 482252 abandoned by Jdlrobson:
Drop Anchor templatePartial usage in Overlays and CtaDrawer

Reason:
Done elsewhere and now in master

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

Jdlrobson closed this task as Resolved.Aug 1 2019, 8:18 PM
Jdlrobson claimed this task.
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a subscriber: Krinkle.