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

Reinstate affiliate links for all eligible DCR articles behind a 0% test #27150

Merged
merged 6 commits into from
May 16, 2024

Conversation

emma-imber
Copy link
Contributor

What does this change?

Reinstates affiliate links for all eligible DCR articles behind a 0% test. We've added a 0% server side test, and we check if a user is in the variant before considering a DCR article published after the cutoff date that isn't on the allowlist. For anyone not in the variant, the allowlist and cutoff date still apply for DCR rendered articles.

We now check that the first two elements in the body content of a DCR article are both paragraph elements. This ensures that the disclaimer doesn't clash with thumbnail images or rich links. We also check that the second paragraph element is over 250 characters. This is to make sure that there is enough text to wrap around the disclaimer if an image was to follow next, so that we don't get unwanted whitespace on the page. These checks should ensure that the DCR version of the disclaimer doesn't negatively impact the UI. These checks apply to articles in the allowlist and before the cutoff date, as they will improve the page experience for everyone.

Screenshots

Disclaimer on mobile:

Screenshot 2024-05-15 at 15 07 12

Affiliate links not inserted to avoid disclaimer clashing with thumbnail image on mobile:

Screenshot 2024-05-15 at 15 15 32

If we didn't have the check, the disclaimer would look like this:

Screenshot 2024-04-04 at 17 13 54

Affiliate links not inserted to avoid disclaimer clashing with thumbnail image on desktop:

Screenshot 2024-05-15 at 15 19 04

If we didn't have this check, the disclaimer would look like this:

Screenshot 2024-04-11 at 15 57 12

@emma-imber emma-imber requested a review from a team as a code owner May 15, 2024 17:08
@SiAdcock SiAdcock requested a review from a team May 16, 2024 09:07
)
def shouldAddAffiliateLinks(content: ContentType)(implicit request: RequestHeader): Boolean = {
val contentHtml = Jsoup.parse(content.fields.body)
val bodyElements = contentHtml.select("body").first().children()
Copy link
Contributor

@SiAdcock SiAdcock May 16, 2024

Choose a reason for hiding this comment

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

Non blocking: I'm not wedded to this suggestion, and I'm not sure of the best place, but would an explanatory comment help?

Suggested change
val bodyElements = contentHtml.select("body").first().children()
val bodyElements = contentHtml.select("body").first().children()
/**
* On smaller devices, the disclaimer is inserted before paragraph 2 of the article body and floats left.
* This logic ensures there are two clear paragraphs of text at the top of the article.
* We don't support inserting the disclaimer next to other element types.
* It also ensures the second paragraph is long enough to accommodate the disclaimer appearing alongside it.
*/

Copy link
Contributor

@dskamiotis dskamiotis left a comment

Choose a reason for hiding this comment

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

Tested thoroughly in CODE and working as expected.

Available documentation on results of testing: here

@emma-imber emma-imber merged commit 0ee270a into main May 16, 2024
3 checks passed
@emma-imber emma-imber deleted the ei/affiliate-links-dcr branch May 16, 2024 13:50
@prout-bot
Copy link
Collaborator

Seen on ADMIN-PROD (merged by @emma-imber 12 minutes and 11 seconds ago)

@prout-bot
Copy link
Collaborator

Seen on FRONTS-PROD (merged by @emma-imber 15 minutes and 28 seconds ago)

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.

4 participants