Page MenuHomePhabricator

Implement first version of read more
Closed, ResolvedPublic5 Story Points

Description

Prerequisite:
There is an extension called Cards that allows rendering of a list of articles (T114393)
API on short term depends on the GettingStarted API, long term we will move this into this extension.

  • The purpose of this task is to tease out UX problems and problems with the design for read more.
  • Show up to 4 article cards after end of article, before Wikipedia footer.
  • If no related cards are available do not show the read more heading
  • Related articles should not impact first paint time.
  • The endpoint should be edge performant and should not cause excess load on the origin servers (??)
  • Articles shown using same backend as apps "Read more" (??)
  • The feature should be featured flagged and turned on only in mobile beta to begin with (see the Timeline Estimate section in T94906).
  • The feature should show up in desktop and should be usable. It does not need to look great (we will work on desktop and the beta feature in the next sprint).
  • It only shows it on pages in the main namespace
  • Title should be Related Article
  • Please consider https://phabricator.wikimedia.org/T113635#1687199


.
Desktop

Cards are identical to those in Gather a MobileFrontend page list and show:

  • page image
  • text extract (first 3 lines of text from article) Wikidata description
  • No arrow at bottom of card

Before implementing as a team it would be useful to discuss how Gather and Read more can share css/templates for such a feature.

Code should live in the RelatedArticles extension and should not interfere with existing code there.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jdlrobson raised the priority of this task from to Normal.Sep 24 2015, 6:02 PM
Jdlrobson added subscribers: KLans_WMF, JKatzWMF, phuedx and 3 others.

@KHammerstein is the wikidata description necessary in a first version? We didn't use it for Gather (I forget reasoning/logic) and this would simply things greatly.

Jdlrobson updated the task description. (Show Details)Sep 24 2015, 6:04 PM
Jdlrobson set Security to None.
Jdlrobson edited a custom field.

@Jdlrobson Yes we can skip wikidata description for now. I'll update mocks and fill additional criteria by tomorrow!

@KHammerstein any reason we can't just use existing Gather design?

Jdlrobson updated the task description. (Show Details)Sep 24 2015, 7:02 PM
Jdlrobson updated the task description. (Show Details)

@Jdlrobson Great point, the designs should be consistent. The design team has been standardizing on a card style that is a bit different from the gather design now - for example the titles aren't overlayed over the image. Would it be possible to update gather so that we can use the same design here?

Sharing the card design between our projects should definitely be an (engineering) goal of this project.

Jdlrobson updated the task description. (Show Details)Sep 25 2015, 3:30 PM
Jdlrobson added a project: RelatedArticles.
Jdlrobson updated the task description. (Show Details)Sep 25 2015, 3:33 PM
Jdlrobson renamed this task from Implement first version of read more in mobile and desktop to Implement first version of read more.Sep 25 2015, 3:38 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson removed KHammerstein as the assignee of this task.
Jdlrobson updated the task description. (Show Details)Sep 25 2015, 5:07 PM
Jdlrobson updated the task description. (Show Details)Sep 29 2015, 4:14 PM
KLans_WMF edited a custom field.Sep 29 2015, 4:22 PM

FYI, we may want to swap out the Gather design for something akin to the search results PageList design in future. If we do this, the same problem applies - the PageList lives in MobileFrontend not RelatedArticles and needs to be shared somehow. Please bear this in mind when implementing so we can easily swap it out cc @KHammerstein @JKatzWMF

Jdlrobson updated the task description. (Show Details)Sep 29 2015, 9:24 PM

Created T114393 to split card creation out as a subtask. After discussion with @phuedx, we came to the conclusion of using a new extension for generating cards with default styling. This will pull code from Gather's card generation.

phuedx added a comment.EditedOct 2 2015, 10:16 AM

Articles shown using same backend as apps "Read more"

AFAICT, the Android app uses Elasticsearch's morelike search feature in order to get suggestions. This is something that the fine, upstanding folk that worked on the GettingStarted extension did too, so there might be an opportunity to break this out too.

phuedx claimed this task.

So I think PageList and the Gather cards (CollectionItem) should probably be the same thing but different variations of each other. I think a Card extension/composer/npm module library will thus help here :)

phuedx added a comment.EditedOct 7 2015, 8:43 AM

I didn't really clock this yesterday, but the mock in T113768 is counter to the requirements at the top:

Cards are identical to those in Gather and show:

  • page image
  • text extract (first 3 lines of text from article)
  • No arrow at bottom of card

In fact, the mock looks like the PageList component/thing.

Change 244149 had a related patch set uploaded (by Phuedx):
[WIP] Add Related Articles section to MobileFrontend

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

phuedx added a comment.Oct 7 2015, 2:38 PM

Yes. AFAIK @jhobs is still working on T114393. I figured I'd wire up the RelatedArticles extension to the WatchstarPageList class to see what was required, which, as it happens, isn't a whole bunch.

phuedx updated the task description. (Show Details)Oct 7 2015, 2:38 PM
phuedx updated the task description. (Show Details)
phuedx updated the task description. (Show Details)Oct 7 2015, 3:22 PM
phuedx added a comment.EditedOct 8 2015, 3:30 PM

@Jdlrobson, @KHammerstein: Where's the mock for the desktop version?

The feature should show up in desktop and should be usable. It does not need to look great (we will work on desktop and the beta feature in the next sprint).

Shall we move this to a follow-on task?

@Jdlrobson @phuedx
Doesn't seem to be a good way to transfer the list style to desktop, even though @JKatzWMF wanted to start with a version close to Read More on apps. And I would really prefer to start using the gather style cards.

Here are 2 versions
1 At bottom of main content area
Uses a gather-style card minus the text extract. The background and border match article table styling.

2 After content area, in footer
Background and border would match gather styling. I prefer this version

Are these technically possible?

Version that is exactly like mobile:

Jdlrobson updated the task description. (Show Details)Oct 13 2015, 4:19 PM
KLans_WMF edited a custom field.Oct 13 2015, 4:24 PM
Jdlrobson updated the task description. (Show Details)Oct 13 2015, 4:25 PM
Jdlrobson updated the task description. (Show Details)
phuedx updated the task description. (Show Details)Oct 13 2015, 4:38 PM
phuedx added a comment.EditedOct 13 2015, 5:23 PM

244149 currently covers all of the AC. However, I'm not happy about the number of requests that it's making (3). I've proposed a shortcut for removing one on the patch.

The WatchstarPageList class makes an additional request to the API to get the watched status of all of the pages by ID. If we could inject our own WatchstarApi-like object at construction time, then we could provide our own stub API, complete with stubbed #load and #isWatchedPage methods, i.e. we could remove the additional request.

phuedx added a comment.EditedOct 15 2015, 11:35 AM

244149 currently covers all of the AC. However, I'm not happy about the number of requests that it's making (3). I've proposed a shortcut for removing one on the patch.

244149 covers all but the Articles shown using same backend as apps "Read more" AC as that's blocked – sorta – by T114916.

The WatchstarPageList class makes an additional request to the API to get the watched status of all of the pages by ID. If we could inject our own WatchstarApi-like object at construction time, then we could provide our own stub API, complete with stubbed #load and #isWatchedPage methods, i.e. we could remove the additional request.

Done in 246463.

I'd appreciate feedback from a couple of keen-eyed reviewers.

There's a lot of crossover between 244149 and @bmansurov's 246137, which is to be expected.

@jhobs / @bmansurov can you take a look at this and try to get to the bottom of what's not working?

dr0ptp4kt updated the task description. (Show Details)Oct 26 2015, 11:40 PM

Change 244149 merged by jenkins-bot:
Add Related Articles section to Minerva

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

Some follow ups needed but this is mostly done:
See https://gerrit.wikimedia.org/r/249035

Would appreciate a reply to:
https://gerrit.wikimedia.org/r/#/c/244149/13/resources/ext.relatedArticles.readMore.minerva/index.js L37

Will sign off sometime tomorrow!
Should be possible to test on http://en.m.wikipedia.beta.wmflabs.org/wiki/Related_test

Jdlrobson updated the task description. (Show Details)Oct 27 2015, 9:27 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson closed this task as Resolved.Oct 27 2015, 9:35 PM

There are a couple of acceptance criteria that I don't understand what they mean nor remember writing. They shouldn't be on this card as this card was simply about getting a first prototype. The latter will probably be solved as part of T116707
(

  • The endpoint should be edge performant and should not cause excess load on the origin servers (??)
  • Articles shown using same backend as apps "Read more" (??)

)

In terms of measuring first paint this is tricky to determine but code-wise it looks fine. We'll do some more vigorous testing later.

I'm gonna sign this off and add these two tasks from the remaining acceptance criteria that I understand:

Nirzar added a subscriber: Nirzar.Nov 4 2015, 9:44 PM