Page MenuHomePhabricator

It should be easier to create new overlays
Closed, ResolvedPublic


Configuring headers

All our overlays follow similar patterns yet we duplicate their templates and their classes.
Let's common up with a generic pattern that allows an overlay to easily configure it's header.

The pain of this was evident when implementing and the required follow up.
In the construction of Weekipedia various patterns emerged which we could use to simplify our codebase and reduce the JS we ship to users.

This patch proposes a new way to generate headers of overlays that is backwards compatible: Hygiene: Allow a more generalised Overlay header

An overlay would have a headings option that can either be an object or a list of headers. This allows us to programatically construct header(s) without having to create a new Hogan template.

To show the benefit I've made use of the new header format in talk overlays: Talk overlay's should not need so many partials

Constraining how we build overlays has the following benefits:

  • All headers have consistent class names and properties (prevents us from class-itis)
  • Markup is identical for all headers
  • Templates should no longer be needed for headers
  • Less code and compile time for Hogan templates (now less of them)
  • Standardises an approach for adding multiple headers to an overlay

Dealing with different screens

in the EditorOverlay and CollectionEditorOverlay in Gather we have logic for various screens e.g. edit, preview, save
Despite this there is no easy way to do this without editing the entire template or hacking the partial by using overlay-sub-header elements

Let's talk about how we could do this better and find a better approach
e.g. Overlay.prototype.screens = { header: headerOptions, content: contentOptions, template: template }



When working on the page issues instrumentation @Niedzielski pointed out that the CleanupOverlay doesn't really do much - it subclasses Overlay and replaces the content area. It renders a list of what are essentially Issues components. It might be a good candidate to look at when starting this refactor work

Related Objects


Event Timeline

Jdlrobson triaged this task as Medium priority.Jul 11 2016, 5:14 PM
Jdlrobson moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.
Jdlrobson renamed this task from Remove the need for an overlay header template partial to It should be easier to customise overlay headers without overriding the entire header partial.Oct 10 2016, 10:06 PM
Jdlrobson claimed this task.
Jdlrobson updated the task description. (Show Details)

Only I have engaged in this discussion so far. I was looking for feedback...

+1 to the benefits listed. I guess the plan would be to create a base overlay header and swap out the existing headers to use the new base header in separate patches?

Essentially we'd move to a composition/options approach and try to have one single template for all overlays.

I explored a bit here for talk overlays:

If the above patch could be modified so you can pass a Header to Overlay.... even better.

new TalkOverlay({
  header: new Header({})
Jdlrobson renamed this task from It should be easier to customise overlay headers without overriding the entire header partial to It should be easier to create new overlays.Jun 2 2017, 8:40 PM
Jdlrobson lowered the priority of this task from Medium to Low.Jun 22 2017, 12:02 AM

For time being this is quite low. We're not building new things. The frontend code is pretty stable.

Jdlrobson added subscribers: nray, pmiazga, Jdrewniak.

@nray @Niedzielski @pmiazga @Jdrewniak This is interesting for a historic/motivation perspective but I'd say actually resolved with changes we've been making to View to support changes like and superseded by the more tangible T212465 and parent tasks.