Page MenuHomePhabricator

[EPIC] Post A-B test technical debt cleanup
Closed, ResolvedPublic

Description

The page issues work has accumulated a lot of technical debt.
If the A/B test shows we need to keep this code and maintain it, it's essential that we make changes to accommodate this change as being permanent.

We will make changes to user edited templates to clarify an expected structure that issues should adhere to for display in mobile.

Objectives

  • Implement a generic solution for all projects per T201975
  • Simplify resources/skins.minerva.scripts/pageIssueParser.js by making changes in associated templates and simplify our code. T206177
  • Simplify icon generation - rather than removing the desktop icon and injecting a new icon for mobile, add the mobile icon in the template T206177
  • Migrate any styles from Minerva via TemplateStyles. (Tracked in T212263.)
  • There is per issue parsing logic in pageIssuesParser.js and group parsing in pageIssues.js. The latter should be separated into pageIssuesParser.js or a new file. There's some related discussion in this patch. (Tracked in T212371)
  • There is view creation and detailed binding logic in pageIssues.js. This should be extracted into pageIssuesView.js / Presenter.js / Renderer.js. Since we're changing the DOM, we should consider moving resources/skins.minerva.content.styles/templates/ambox.less to something more page issues specific. (Tracked in T212376)
  • pageIssues.js has unclear responsibilities in name. It seems to be all about formatting which requires parsing an input DOM into a model and then rendering the result. We should rename it to pageIssuesFormatter.js so its responsibilities are as clear as possible. (Also covered in T212376.)

Event Timeline

Jdlrobson triaged this task as Normal priority.Aug 9 2018, 5:36 PM
Jdlrobson created this task.
Jdlrobson edited projects, added Technical-Debt; removed Epic.
Jdlrobson changed the task status from Open to Stalled.Aug 9 2018, 9:27 PM

Blocked on T200792

Jdlrobson updated the task description. (Show Details)Aug 17 2018, 12:07 AM
Jdlrobson changed the task status from Stalled to Open.Oct 3 2018, 10:15 PM
Jdlrobson renamed this task from Post A-B test technical debt cleanup to [EPIC] Post A-B test technical debt cleanup.Oct 3 2018, 10:47 PM
Jdlrobson added a project: Epic.
Jdlrobson moved this task from Needs Prioritization to Epics/Goals on the Readers-Web-Backlog board.
  1. Do we need a spike to nail down the canonical template format and generate on-wiki examples for the community (and us) to use (hopefully on more than just enwiki)? For example, what should the multiple issues template look like? @Jdrewniak's pages for testing (page issues inventory, ambox templates) were essential but if they had been on wiki we could have executed the page issue code itself on them.
  2. There is per issue parsing logic in pageIssuesParser.js and group parsing in pageIssues.js. The latter should be separated into pageIssuesParser.js or a new file. There's some related discussion in this patch.
  3. There is view creation and detailed binding logic in pageIssues.js. This should be extracted into pageIssuesView.js / Presenter.js / Renderer.js. Since we're changing the DOM, we should consider moving resources/skins.minerva.content.styles/templates/ambox.less to something more page issues specific.
  4. pageIssues.js has unclear responsibilities in name. It seems to be all about formatting which requires parsing an input DOM into a model and then rendering the result. We should rename it to pageIssuesFormatter.js so its responsibilities are as clear as possible.
  5. All the formatting seems like something that should run server side if it's necessary to exist to avoid intense reflows. If it's JavaScript, it could move to PCS. If it's PHP, it can live in MobileFormatter. Ideally, most of this code can disappear with changes to the templates. The current parsing approach is unsustainable.
  6. The icons seem on the boundary of wiki content and chrome. We use severity for color and icon styling but I wonder if there's room in TemplateStyles for per skin customization. Is there any way to move these assets and styles to the templates themselves?

My preference is to work towards removing all formatting and parsing by moving logic to the templates via data attributes. Once this happens we can give editors a clear call to action to fix their stuff (check out T206177: Provide more sustainable support for severity)

We are limited on time so making it possible for our code as dumb as possible seems like the best goal for now. We can revisit later when community has hopefully improved their templates.

I agree and would take it a step further to say that we should gracefully degrade to the default presentation when data attributes are not present. This means our parser handles exactly the cases we expect and otherwise does nothing. The code becomes small, testable, and maintainable and the presentation is either exactly what we've carefully specified or exactly what the editor has specified. In the case of data attributes, it's both.

In my opinion, it would be foolish to embark on any further changes without a perfectly clear plan and test strategy both noted in #1.

#5 and #6 seem lofty but the rest are doable in my view.

Quiddity removed a subscriber: Quiddity.Oct 4 2018, 10:35 PM
Jdlrobson updated the task description. (Show Details)Oct 10 2018, 10:25 PM

with regards to #1 I don't think so. Templates are living documents. Any spike we do now will be redundant next time there's a community-led redesign of page issues. We don't own this content, so we should stop acting like we do. To me, what that means is when we meddle with this content, we should be checking it has explicitly opted in, whether that be via some kind of data attribute or class name. If it doesn't work that's on the template editor to fix, not us. With regards to using MobileFormatter, I will not allow that as the maintenance cost on the server seems even higher to me given the impact of cached HTML.

Out of everything, my biggest concern is with the issueParser code as its the most prone to breakage and hence why I'm focusing on finding a better solution for T206177

I am being told we don't have much focus time to wrap up this project, so at the very least I want to get that addressed, as the others seem more clearly defined and can be chipped away.

I agree and would take it a step further to say that we should gracefully degrade to the default presentation when data attributes are not present.

I like this idea and that was compatible with my view in T206177. I think we'd need to use console.warn to communicate to editors they need to opt in to give them time to do so before we remove the surrounding code.

Although I still think we should load our styles regardless of the data attribute.

phuedx removed a subscriber: phuedx.Oct 11 2018, 11:43 AM
Nirmos removed a subscriber: Nirmos.Nov 12 2018, 10:45 PM

It wasn't clear what the status of this task was as all subtasks appear to be closed.

There is per issue parsing logic in pageIssuesParser.js and group parsing in pageIssues.js. The latter should be separated into pageIssuesParser.js or a new file. There's some related discussion in this patch.

@Jdrewniak, I think this is what you were talking about doing as a possible next step? If so, please make a subtask here.

  • There is view creation and detailed binding logic in pageIssues.js. This should be extracted into pageIssuesView.js / Presenter.js / Renderer.js. Since we're changing the DOM, we should consider moving resources/skins.minerva.content.styles/templates/ambox.less to something more page issues specific.
  • pageIssues.js has unclear responsibilities in name. It seems to be all about formatting which requires parsing an input DOM into a model and then rendering the result. We should rename it to pageIssuesFormatter.js so its responsibilities are as clear as possible.

These bullets are incomplete.

I'm not sure about the other items.

Jdlrobson updated the task description. (Show Details)Dec 17 2018, 6:54 PM

Migrate any styles from Minerva via TemplateStyles.

This will happen slowly and post-deploy.

There is per issue parsing logic in pageIssuesParser.js and group parsing in pageIssues.js. The latter should be separated into pageIssuesParser.js or a new file. There's some related discussion in this patch.

There is view creation and detailed binding logic in pageIssues.js. This should be extracted into pageIssuesView.js / Presenter.js / Renderer.js. Since we're changing the DOM, we should consider moving resources/skins.minerva.content.styles/templates/ambox.less to something more page issues specific.

pageIssues js has unclear responsibilities in name. It seems to be all about formatting which requires parsing an input DOM into a model and then rendering the result. We should rename it to pageIssuesFormatter.js so its responsibilities are as clear as possible.

Are the other bullets up to date @Niedzielski or do they no longer apply? I must confess I'm a bit lost with what we are satisfied about in the page issues code.

@Niedzielski is going to spin out some tasks from this tagged "technical debt" and we'll close the epic.

Niedzielski closed this task as Resolved.Dec 19 2018, 11:54 PM
Niedzielski claimed this task.
Niedzielski updated the task description. (Show Details)
Niedzielski updated the task description. (Show Details)