Page MenuHomePhabricator

Have a consistent way of doing Views within Views
Closed, DuplicatePublic

Description

Sometimes we need to put a View inside another View. A good example is the Icon class which sometimes gets embedded in a template using

{{{icon}}}

And sometimes as

{{#fooIcon}}{{>icon}}{{/fooIcon}}

We need a consistent way of embedding these. Right now the existence of two ways causes confusion in code review.

Developer notes

We discussed this in the CR session on 17th August 2017

As @pmiazga rightly pointed out in the TalkOverlay we create views in 3 different ways - template data e.g. headerButtons raw HTML (e.g.heading), using templatePartials and passing the options of a View class (footerAnchor).

There's also a 4th way (toHtmlString)

The ability to use template partials adds to this problem. We should standardise on one approach and stick to it.

Related: T149909

Event Timeline

Jdlrobson raised the priority of this task from to Needs Triage.
Jdlrobson updated the task description. (Show Details)
Jdlrobson added a project: Readers-Web-Backlog.
Jdlrobson moved this task to Triaged but Future on the Readers-Web-Backlog board.
Jdlrobson added a subscriber: Jdlrobson.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 26 2015, 4:50 PM
Jdlrobson triaged this task as Low priority.Sep 16 2015, 6:57 PM
Jdlrobson edited projects, added MobileFrontend; removed Readers-Web-Backlog.
Jdlrobson set Security to None.
Jdlrobson updated the task description. (Show Details)Jun 6 2017, 9:55 PM
Niedzielski added a subscriber: Niedzielski.

What are the pros and cons of each? {{{icon}}} seems clear and concise.

Jdlrobson updated the task description. (Show Details)Aug 17 2017, 5:32 PM
Jdlrobson added a subscriber: pmiazga.

Using

{{{icon}}}

and

{{iconOptions}}{>icon}{{/iconOptions}}

are both flawed in that they only copy across the HTML and the Icon is stateless. Doing this means any event handlers need to be redefined by the thing using the class.

I think the approach most compatible with how we are doing things is to use the postRender method to update the template
e.g. template would look like this:

<div><Icon data-key='foo' /></div>

Code:

postRender: function () {
  this.$('.icon[data-key=foo]').replaceWith( new Icon( { name: foo } ) )
}

I think long term, we'd look to avoid partials altogether (or at least abandon the use of template inheritance)
e.g.

function TalkOverlayContent() {
}
TalkOverlayContent.prototype = {
  template: 'TalkOverlayContent.hogan'
}

var overlay = new Overlay( { content: new TalkOverlayContent(), header: new OverlayHeader() } );

Does this seem like a reasonable approach?

Jdlrobson raised the priority of this task from Low to Normal.Aug 30 2018, 1:33 AM