search

guardian / facia-scala-client

Low level client for the Facia JSON API
Other
2 stars 1 forks source link

Add a backfillLimit to the collection config model. #182

Closed lmath closed 7 years ago

lmath commented 7 years ago

Currently, emails fronts created in the fronts tool can have containers with backfill but then in frontend we restrict the number of items in a backfill-ed container to 6.

It is going to be required to be able to specify the number of items in the containers. There will be a related PR in the fronts tool to specify this value, but for this to be possible, inevitably the config model for collections needs to have an optional backfillLimit field.

davidfurey commented 7 years ago

Looks good to me @janua ?

janua commented 7 years ago

@lmath Apart from the benefit from not requesting too many items, why is this required?

davidfurey commented 7 years ago

It is purely so that there can be editorial control over how many items there are in each container on email fronts.

janua commented 7 years ago

@davidfurey Why only just include backfill if this is for limiting items? I think if you are going to do this you need to make it very clear in facia-tool that this only limits the amount of items pulled in by backfill.

This then needs to be enforced in this library otherwise you will have to do it in every project that uses the library!

We once went down the path of allowing editorial to decide how many items a container could have; but ended up not using it as there were too many places we had to enforce it and a lot of edge cases too. The difference for us was that we tried to include normal curated items in it too, and that's where the edge cases really begun to show up.

I just want to be clear exactly what this is and how it's going to be used!

joelochlann commented 7 years ago

Yep fair point @janua. For non-email fronts, the number of displayed cards is limited at the point of rendering by the choice of layout (at least for web fronts, not sure about the app). But for email fronts, we have a small selection of layouts which don't specify how many cards can appear. This is fine for curated collections. But when backfill is involved, we need a way to limit the number of cards appearing.

Arguably the naming is misleading here - it's not really a limit on the backfill (implying changes to facia-press), it's just a rendering detail. Maybe call it maxCardsToDisplay? (It would impose a display limit on the total of curated + backfill but I ran that by Celine and she thinks that's sensible functionality)

davidfurey commented 7 years ago

@janua and @joelochlann : I've made the changes we discussed

janua commented 7 years ago

Great! Makes much more sense now 👍 ❤️