Page MenuHomePhabricator

Reduce Cards confusion by merging it into RelatedArticles extension
Closed, ResolvedPublic

Description

The Cards extension is confusing, neglected and there are problems with the documentation. We will resolve this by consolidating the code with RelatedArticles

Acceptance criteria

Original task

Given that I include cards through the mw.cards.CardsGateway
When I show the cards
Then I expect the cards to show the same images as those I get from pageimages

That doesn't work as expected.

Note the code example at Extension:Cards#How to Use is

mw.loader.using( 'ext.cards' ).done( function () {
	var gateway = new mw.cards.CardsGateway( { api: new mw.Api() } );
	// 'Book' and 'Phone' are page titles, 200 is the thumbnail width in pixels
	gateway.getCards( ['Book', 'Phone'], 200 ).done( function( cards ) {
	    $( '#bodyContent' ).append( cards.$el );
	} );
} );

This leads to a api-call like this, but that has a continue value set, which is not resolved before getCards() returns. Ie. the whole set is not downloaded, only the initial part is, and thus only a single card has an image.

Either getCards() must return a resolved set or the extension page must describe how the developer can resolve the full set herself. In the present state the example is incomplete

All of the cards should have images, but only one does.
The usual proof that it did actually happen! :)

Event Timeline

jeblad updated the task description. (Show Details)

Seems like the api query fired by cards needs a pilimit like the length of the provided array. It does set exlimit. Otherwise it will not work. It is now left out.

Note that the length of the array can still be larger than the limit enfoced by the api itself (50 entries?) and that this can happen on any of the lists, so getCards() should be ready to handle continue anyhow.

Jhernandez added a project: RelatedArticles.
Jhernandez added a subscriber: Jhernandez.

How haven't we noticed this before?

Note that on the related articles this seems to be handled.

dr0ptp4kt triaged this task as Medium priority.Jun 8 2016, 4:21 PM

Seems like RelatedArticles is not using the CardsGateway.

Besides fixing it, we may want to use it too in RelatedArticles instead of some custom api call.

jhobs added a subscriber: jhobs.

I'll be creating a new task for the RelatedArticles hygiene portion and editing this comment once it has been done.

RelatedArticles has changed (or is planned to change) to not rely on Cards, and this comment is no longer valid.

Why not do it as part of this? Make related articles actually use Cards (and fix the cards impl)?

@bmansurov any insight on why RelatedArticles didn't use Cards implementation?

@Jhernandez by comparing the two gateways (Cards vs RelatedArticles) I see the Cards gateway is not flexible to cater for RelatedArticles needs. I suppose that was the reason.

Jdlrobson added a subscriber: Jdlrobson.

@bmansurov @Jhernandez what needs to happen here? Should we remove this code in Cards or should we make RelatedArticles use the code in Cards. We've talked about merging the two extensions so I'm leaning towards the former..

@Jhernandez by comparing the two gateways (Cards vs RelatedArticles) I see the Cards gateway is not flexible to cater for RelatedArticles needs. I suppose that was the reason.

Could you expand on this? What's missing?

@bmansurov @Jhernandez what needs to happen here? Should we remove this code in Cards or should we make RelatedArticles use the code in Cards[?] We've talked about merging the two extensions so I'm leaning towards the former[.]

👍 Merge Cards into RelatedArticles and deprecate/remove the Cards extension either by:

  • Making Cards a library that we bundle into RA.
  • Moving the Cards RL modules into RA – this is likely easier but would mean that folk can't consume the Cards code without having RA loaded.

I'd vote for Moving the Cards RL modules into CardsRelatedArticles – this is likely easier but would mean that folk can't consume the Cards code without having RA loaded.

RelatedArticles will be available but disabled by default, but it should still be possible to load the modules if something else wants them, right?

Jdlrobson renamed this task from Cards lacks images in some (?) cases to Reduce Cards confusion by merging it into RelatedArticles extension.May 4 2017, 6:26 PM
Jdlrobson updated the task description. (Show Details)

^ how about that as a proposal?

Change 357519 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Cards@master] Empty the Cards extension

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

Change 357520 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/RelatedArticles@master] Migrate Cards code to RelatedArticles

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

@Jdlrobson needs some more work. I've inverted the depends-on relationship so that the migration patch is forced to be first, and then and only then we will be able to remove the cards code.

Change 357520 merged by jenkins-bot:
[mediawiki/extensions/RelatedArticles@master] Migrate Cards code to RelatedArticles

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

Change 357519 merged by jenkins-bot:
[mediawiki/extensions/Cards@master] Empty the Cards extension

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

Jdlrobson added a subscriber: ABorbaWMF.

@ABorbaWMF we'll appreciate a quick review of the Related Articles feature and check it's behaving per normal. If you're too busy I think one of the devs can do that.

Quoting @Reedy's review of rECDS3ab1a1c5b788: Empty the Cards extension:

Aren't we going to end up causing problems with WMF config still using it...

I replied on irc and the answer is no. Cards is an inert extension for the next week. We will then remove it.