Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add next and previous buttons to image content in a series #20462

Merged
merged 11 commits into from
Nov 1, 2018

Conversation

philmcmahon
Copy link
Contributor

@philmcmahon philmcmahon commented Oct 3, 2018

What does this change?

Modifies the lightbox for image content to add next and previous buttons.

This is achieved via the next/previous functionality added to the content api. When someone opens the light box, if the content is of image type and there's only one image available then a request is made for the next/previous 100 images in the same series.

I wrote all the javascript so there's probably a lot wrong with it...

Screenshots

screen shot 2018-10-03 at 14 53 25

Here's a video showing the network requests on an example with https://www.theguardian.com/commentisfree/picture/2022/may/22/brian-adcock-on-the-looming-sue-gray-report-cartoon :

Screen.Recording.2022-06-08.at.14.32.18.mov

What is the value of this and can you measure success?

We've got tracking on the attention time of images, we also know when people have opened a lightbox, so something like 'difference between attention time for people who open the lightbox before and after this change'

Tested

  • Locally
  • On CODE (optional)

@PRBuilds
Copy link

PRBuilds commented Oct 3, 2018

PRbuilds results:

Screenshots
wide.pngdesktop.pngtablet.pngmobile.png

💚 Exceptions
thrown-exceptions.js

💚 A11y validation
a11y-report.txt

💚 Microdata Validation
microdata.txt

Apache Benchmark Load Testing
loadtesting.txt

LightHouse Reporting
lighthouse.html

--automated message

@paperboyo
Copy link
Contributor

💎× 💎

I hope it’s just a beginning, a beginning of a Neverending Story. Because whenever we present readers with any kind of ending, we are asking them to leave. And they do. This is wrong.

This has been 6 years in the thinking. @philmcmahon is wholly credited with actually making this happen, @mchv for the other crucial piece. @regiskuckaertz for another (how to increase the limit not to 200, but, simply, to all?). Thank you all, but Phil – twice!

Now, for something incredibly pathetic, but I want to mention some (not all!) the people I bored with this over the years (to the point that most of them aren’t here any more): @gklopper (“it’s Latin civilisation: all textual content is one vertical scroll from top to bottom, all multimedia – left-to-right”), @theefer, @phamann, @rich-nguyen @jamesgorrie, @mattosborn, @akash1810, @currysoup, @blishen, @hoyla. Terribly, terribly sorry for the spam, I am just a little happy right now.

Current, initial, implementation (test here) should not be controversial. Nor are there any UX/UI dilemmas to solve (just yet). Apart from where one lands on Lightbox close (we opted for the easier answer here).

Next are all galleries, next – videos, next – content beyond series, next – starting with any in-article image, next – landing on something unexpected but related, next – making informed turns in a journey, next – a browser for this here viewer, next – all textual content, next, next, next, next…

What is the value of this and can you measure success?

Don’t care much for using numbers as justification for stuff that’s obvious. But this approach should prolong session time (a tiny bit). If we cannot turn that into advantage, let’s close shop.

): Promise<Object> {
const pathPrefix = direction === 'forwards' ? 'getnext' : 'getprev';
const seriesTag = config
.get('page.nonKeywordTagIds')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this ever be null or undefined? Might be worth specifying a default of '' (empty string), as the second parameter to this function call.

}))
.then(nextPrevJson => {
// combine this image, and the ones before and after it, into one big array
const allImages = nextPrevJson.previous
Copy link
Contributor

@SiAdcock SiAdcock Oct 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could destructure the arguments of this callback:

.then(({ next, previous }) => {
  const allImages = previous
  // ...
})

GalleryLightbox.loadNextOrPrevious(currentId, 'forwards'),
GalleryLightbox.loadNextOrPrevious(currentId, 'backwards'),
])
.then(result => ({
Copy link
Contributor

@SiAdcock SiAdcock Oct 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could destructure this result array

.then(([nextResult, previousResult]) => ({
  next: nextResult.map(e => e.images[0])
  // ...
})

Promise.all([
GalleryLightbox.loadNextOrPrevious(currentId, 'forwards'),
GalleryLightbox.loadNextOrPrevious(currentId, 'backwards'),
])
Copy link
Contributor

@SiAdcock SiAdcock Oct 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naive question time 😊

Will this happen every time the lightbox is opened? Is there a mechanism to prevent us from loading next and previous images that have already been loaded?

Perhaps it would be better to move some of this logic into the FSM.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The galleryJson.images.length < 2 will prevent this from running if the images have already been loaded, but maybe more stuff could be done in the FSM hmm I'll take a look

@mchv
Copy link
Member

mchv commented Oct 4, 2018

@philmcmahon one think which I think may be broken, or work differently, is attention time. Ophan & Datalake expect attention time to be bound to a Content API id and therefore to a page. When loading asynchronously another piece of content into an existing page, attention time will increase for the initial page, unless, I think, you update orphan tracker to handle this case properly.

@paperboyo
Copy link
Contributor

paperboyo commented Oct 4, 2018

attention time

How does the tracker (dis)count idle time? Scrolling? Will a stationary mouse pointer mindlessly clicking one button count as spent attention? How about just swiping?

@mchv
Copy link
Member

mchv commented Oct 4, 2018

@paperboyo we use the strategy from upworthy, see code, which is quite similar to parse.ly one and AMP engagement measurement.


if (this.galleryJson && galleryJson.id === this.galleryJson.id) {
loadOrOpen(newGalleryJson: Object): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth typing this to avoid using Object?

We have the GalleryJson type that is used in this file, hopefully this overlaps with the structure of the newGalleryJson? 😁😁

type GalleryJson = {
    id: string,
    headline: string,
    shouldHideAdverts: boolean,
    standfirst: string,
    images: Array<ImageJson>,
};

@philmcmahon philmcmahon force-pushed the pm-nextprev-simple branch 2 times, most recently from 5bc03ff to 89d4ed3 Compare October 10, 2018 16:21
@SiAdcock
Copy link
Contributor

SiAdcock commented Oct 16, 2018

@mchv

When loading asynchronously another piece of content into an existing page, attention time will increase for the initial page, unless, I think, you update orphan tracker to handle this case properly.

A similar question cropped up when we were discussing using the YouTube end slate to handle onward journeys from videos instead of our own end slate. Clicking the YouTube end slate would keep the user on the same page but show a new video in place, increasing the attention time for the page, but effectively as a result of the user watching a different piece of content.

From Stuart Mutter:

I'm imagining these will be edge cases and we tend to report on median attention time which is less affected by massive outliers.
if we start noticing large increases in attention time though we will definitely investigate further (looking at multiple videos first!)

Perhaps users scrolling through images would be considered outliers in the same vain?

@philmcmahon philmcmahon force-pushed the pm-nextprev-simple branch 2 times, most recently from a910220 to 5e31727 Compare October 30, 2018 16:10
@philmcmahon
Copy link
Contributor Author

@SiAdcock I think I've sorted things out as we discussed, when you get a chance let me know what you think! Essentially fetchSurroundingJson has become just a function that returns a promise, which is called by init before loading the lightbox.

Copy link
Contributor

@SiAdcock SiAdcock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks much clearer, thanks for the refactor!

@philmcmahon philmcmahon merged commit 6318464 into master Nov 1, 2018
@philmcmahon philmcmahon deleted the pm-nextprev-simple branch November 1, 2018 12:29
@prout-bot
Copy link
Collaborator

Seen on PROD (merged by @philmcmahon 16 minutes and 49 seconds ago)

@paperboyo
Copy link
Contributor

OMG! Thank you!

Just because I couldn’t help with code, I spent last couple of weeks upgrading image resolution on an abandoned series of best daily photographs. Some beautiful, some funny, some terrifying shots there good for testing this feature!

Currently, to see the whole series (go beyond a limit of 50), easiest is to Google the caption of the last photo to pick up the stream further in.

ioannakok added a commit to guardian/content-api-scala-client that referenced this pull request Jun 8, 2022
We're trying to support LightBox functionality that was introduced here: guardian/frontend#20462

Co-authored-by: Roberto Tyley <[email protected]>
rtyley added a commit to guardian/content-api-scala-client that referenced this pull request Jun 9, 2022
We're trying to support LightBox functionality that was introduced here: guardian/frontend#20462

Co-authored-by: Roberto Tyley <[email protected]>
ioannakok added a commit that referenced this pull request Jun 9, 2022
rtyley added a commit to guardian/content-api-scala-client that referenced this pull request Jun 10, 2022
Get rid of Aux[Q,R]

Allow specifying direction to get next search query

We're trying to support LightBox functionality that was introduced here: guardian/frontend#20462

Co-authored-by: Roberto Tyley <[email protected]>

Support direction for all paginatable queries, and remove unused PaginatedApiResponse classes
rtyley added a commit to guardian/content-api-scala-client that referenced this pull request Jun 10, 2022
Get rid of Aux[Q,R]

Allow specifying direction to get next search query

We're trying to support LightBox functionality that was introduced here: guardian/frontend#20462

Co-authored-by: Roberto Tyley <[email protected]>

Support direction for all paginatable queries, and remove unused PaginatedApiResponse classes
rtyley added a commit to guardian/content-api-scala-client that referenced this pull request Jun 10, 2022
Get rid of Aux[Q,R]

Allow specifying direction to get next search query

We're trying to support LightBox functionality that was introduced here: guardian/frontend#20462

Co-authored-by: Roberto Tyley <[email protected]>

Support direction for all paginatable queries, and remove unused PaginatedApiResponse classes
rtyley added a commit to guardian/content-api-scala-client that referenced this pull request Jun 10, 2022
Get rid of Aux[Q,R]

Allow specifying direction to get next search query

We're trying to support LightBox functionality that was introduced here: guardian/frontend#20462

Co-authored-by: Roberto Tyley <[email protected]>

Support direction for all paginatable queries, and remove unused PaginatedApiResponse classes
rtyley added a commit to guardian/content-api-scala-client that referenced this pull request Jun 10, 2022
Get rid of Aux[Q,R]

Allow specifying direction to get next search query

We're trying to support LightBox functionality that was introduced here: guardian/frontend#20462

Co-authored-by: Roberto Tyley <[email protected]>

Support direction for all paginatable queries, and remove unused PaginatedApiResponse classes
ioannakok added a commit that referenced this pull request Jun 22, 2022
rtyley added a commit that referenced this pull request Jun 30, 2022
rtyley pushed a commit that referenced this pull request Jun 30, 2022
The latest version of the CAPI client (v19) includes
guardian/content-api-scala-client#359,
which we can use to replace the pagination code based on `play-iteratees`
(https://github.com/playframework/play-iteratees - introduced in July 2014
with #5199), currently blocking the
Scala 2.13 upgrade for Frontend (see #24817).

The updated CAPI client is binary-incompatible with previous versions of
the CAPI client, meaning we also need to update the FAPI client as it also
uses CAPI.

The updates to the CAPI client also support `previous`/`next` search
functionality, as used by the Lightbox, meaning that the small
`ContentApiNavQuery` hack for the Lightbox (introduced here:
#20462) can now be removed.

A similar PR updating FAPI & CAPI in Ophan is at
guardian/ophan#4719.

Co-authored-by: Roberto Tyley <[email protected]>

We're adding a test that will break at build time rather than runtime if
our versions of the FAPI & CAPI clients are incompatible.

See also:

* #25139 (comment)
* https://github.com/guardian/ophan/pull/4719/files#diff-ee7f97c92065084bba37d70d043ad0daa0d7745f235d0ad3206b59f073829529
rtyley added a commit that referenced this pull request Jun 30, 2022
The latest version of the CAPI client (v19) includes
guardian/content-api-scala-client#359,
which we can use to replace the pagination code based on `play-iteratees`
(https://github.com/playframework/play-iteratees - introduced in July 2014
with #5199), currently blocking the
Scala 2.13 upgrade for Frontend (see #24817).

The updated CAPI client is binary-incompatible with previous versions of
the CAPI client, meaning we also need to update the FAPI client as it also
uses CAPI. A similar PR updating FAPI & CAPI in Ophan is at
guardian/ophan#4719. Like there, we're adding a
test that will break at build time rather than runtime if our versions of
the FAPI & CAPI clients are incompatible.

The updates to the CAPI client also support `previous`/`next` search
functionality, as used by the Lightbox, meaning that the small
`ContentApiNavQuery` hack for the Lightbox (introduced here:
#20462) can now be removed.

See also:

* #25139 (comment)
* https://github.com/guardian/ophan/pull/4719/files#diff-ee7f97c92065084bba37d70d043ad0daa0d7745f235d0ad3206b59f073829529

Co-authored-by: Roberto Tyley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants