Page MenuHomePhabricator

Popups should not use moment.js for a single line
Closed, ResolvedPublic1 Story Points

Description

Story

As a reader, I want Page Previews to use as little data as possible so that it loads faster and is less costly.

Acceptance Criteria

  • moment.js and its traces are removed from the codebase.
  • The last updated date does not appear on a preview.
  • The cog is placed on the right for LTR previews (see the mock below).
  • The cog is placed on the left for RTL previews.

Mocks


Background (by @Jdlrobson)

Remove the use of moment. 48.8kb simply to render the line "Edited 33 minutes ago" is unacceptable. Consider using the library in MobileFrontend and i18n messages which are considerably smaller.

On a clear cache, after gzipping, Page Previews increases the page JS from 192kb to 222kb. It's only 207kb without the moment library.

Removing this code would improve the load time of the first hovercard, making them feel much more responsive to an end user.

We use a technique in MobileFrontend to achieve the same goal for a lot fewer bytes. I would recommend we leverage that.

Event Timeline

Restricted Application added a subscriber: Zppix. Β· View Herald TranscriptJun 14 2016, 7:26 AM
Jdlrobson raised the priority of this task from Normal to Needs Triage.Jun 14 2016, 7:26 AM
Jdlrobson removed projects: Epic, Readers-Web-Backlog.
β€’ jhobs triaged this task as Normal priority.Jun 15 2016, 3:17 PM
β€’ jhobs moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.

We should consider removing the datetime stamp altogether, as well.

β€’ jhobs added a subscriber: β€’ jhobs.

We should consider removing the datetime stamp altogether, as well.

Tagging Design for this inquiry.

Agabi10 added a subscriber: Agabi10.Sep 1 2016, 5:39 PM
Jdlrobson added a subscriber: ovasileva.EditedSep 26 2016, 11:50 PM

@ovasileva @bmansurov - to be clear although this is tech debt, it's also something we should prioritise from a product perspective.

This should block rolling out Hovercards and be high priority. 50kb is a lot for every page view and this is easily fixed.

On a clear cache, after gzipping, Hovercards bloats the page JS from 192kb to 222kb. It's only 207kb without the moment libray.

Removing this code would improve the load time of a hovercard, making them much more responsive and would reduce the likelihood that the performance team rolls back our release due to performance regressions.

I'm not sure why this is in "needs analysis". We use a similar technique in MobileFrontend to achieve the same goal for a lot less bytes. We should leverage that.

(in other words half of the JavaScript code for the Popups extension renders a line that says "Edited 33 minutes ago")

This comment was removed by phuedx.

^ I'm not sure what happened there…


@Jdlrobson: Could you update the description to include the value of this work, which might seem intuitive but is worth stating, and solutions that've been proposed (T137775#2382700 and T137775#2669404).


Removing this code would improve the load time of a hovercard, making them much more responsive…

To be clear, removing this code (or replacing it with a smaller chunk of code) would improve the load time of the first hovercard.

… and would reduce the likelihood that the performance team rolls back our release due to performance regressions.

It's true that there's a non-zero chance that Performance can roll back one of our changes but this risk is vague and hard to quantify – to me, it makes "performance" as much of a bogeyman as "technical debt". One way to quantify this risk is to ask Performance for a set of guidelines (better yet, rules) as to what they consider a critical regression.

I'm not sure why this is in "needs analysis". We use a similar technique in MobileFrontend to achieve the same goal for a lot less bytes. We should leverage that.

At least, the following questions – explicit and implicit – remain unanswered:

  • Design: does the user need to know when the page was last edited?
    • If yes, then are there other ways we could signal to the user that the page was edited recently?
  • Is the MobileFrontend-solution reusable in the Popups extension?
    • If no, then how can it be made to be reusable?
      • If there's more than one approach, then how complex/risky are they?
Jdlrobson updated the task description. (Show Details)Sep 27 2016, 6:15 PM

^ I'm not sure what happened there…

@Jdlrobson: Could you update the description to include the value of this work, which might seem intuitive but is worth stating, and solutions that've been proposed (T137775#2382700 and T137775#2669404).

Removing this code would improve the load time of a hovercard, making them much more responsive…

To be clear, removing this code (or replacing it with a smaller chunk of code) would improve the load time of the first hovercard.

@phuedx: What do you mean by on the first?

… and would reduce the likelihood that the performance team rolls back our release due to performance regressions.

It's true that there's a non-zero chance that Performance can roll back one of our changes but this risk is vague and hard to quantify – to me, it makes "performance" as much of a bogeyman as "technical debt". One way to quantify this risk is to ask Performance for a set of guidelines (better yet, rules) as to what they consider a critical regression.

I'm not sure why this is in "needs analysis". We use a similar technique in MobileFrontend to achieve the same goal for a lot less bytes. We should leverage that.

At least, the following questions – explicit and implicit – remain unanswered:

  • Design: does the user need to know when the page was last edited?

From a product standpoint, while I wouldn't consider this crucial, it is a nice to have, especially for logged-in users.

    • If yes, then are there other ways we could signal to the user that the page was edited recently?
  • Is the MobileFrontend-solution reusable in the Popups extension?
    • If no, then how can it be made to be reusable?
      • If there's more than one approach, then how complex/risky are they?

To be clear, removing this code (or replacing it with a smaller chunk of code) would improve the load time of the first hovercard.

@phuedx: What do you mean by on the first?

@ovasileva: What @phuedx means is that the relevant code is requested once the first link is hovered during a page session. Once that code is loaded it doesn't need to be loaded again, so every subsequent hovercard is loaded faster. Thus, smaller code payload means faster loading of the first hovercard.

Of course, if the overall loading time is less than the hovercard activate time, then it won't be noticed (so, slower connections see a bigger improvement).

Krinkle removed a subscriber: Krinkle.
Nirzar added a subscriber: Nirzar.Oct 6 2016, 10:02 PM

Finally, got around evaluating this. from design's side the purpose of the tagline was to show how fresh the article content is. but considering the high cost, the value seems minor, since it's not proven to actually show how fresh the content is. it's an estimate.

We suggest to get rid of this line and move the cog in a consistent right position.

let's save some bytes

@ovasileva over to you!

ovasileva added a comment.EditedOct 7 2016, 11:11 AM

@Nirzar - sounds good, I agree for now. However, I'd like to bring this back at some point, as it's one of the selling points for editors in terms of hovercards vs. navpopups. Editing task description and moving to triaged but future

ovasileva updated the task description. (Show Details)Oct 7 2016, 11:14 AM
bmansurov updated the task description. (Show Details)Oct 14 2016, 4:29 PM
Nirzar lowered the priority of this task from Normal to Low.Dec 20 2016, 5:44 PM
ovasileva raised the priority of this task from Low to High.Jan 26 2017, 2:39 PM
ovasileva moved this task from Triaged but Future to Upcoming on the Readers-Web-Backlog board.
phuedx updated the task description. (Show Details)Jan 26 2017, 2:54 PM
ovasileva set the point value for this task to 1.Feb 1 2017, 6:44 PM

Change 335482 had a related patch set uploaded (by Jdlrobson):
Remove last modified line

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

36 minutes after sprint planning!

Change 335482 merged by jenkins-bot:
Remove last modified line

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

phuedx assigned this task to Nirzar.Feb 2 2017, 2:05 PM

@Nirzar: @Jdlrobson's change has been deployed to our staging server. You can see it in action here: http://reading-web-staging.wmflabs.org/w/index.php?title=Category:Popups_corpus. Make sure that you take a look at the preview for the "Rtl popups" page, which exercises the layout of a preview when the page's content language is displayed RTL.

Nirzar added a comment.Feb 2 2017, 9:27 PM

Looks good. can we summarize the performance benefit in detail? it would be handy to justify removing a feature.

ovasileva closed this task as Resolved.Feb 3 2017, 1:42 PM