[NASAA-203/204] - Create AmpExperiment component, add top stories experiment to PS articles #12048

hotinglok · 2024-10-14T16:44:42Z

Resolves NASAA-203 and NASAA-204
Experiment Brief

Overall changes

Implements experimentation on AMP pages via AMP's built-in experimentation tools. Created the <AmpExperiment/> component and a config used for the NASAA Google Discover experiment. This PR also adds front-end changes based on the variants served by the config.

The front-end changes consist of inserting an edited copy of the Top Stories section into the blocks array that are always hidden and only shown on one column breakpoints for specific variants via css. The desktop behaviour for all variants should behave the same as usual, only showing the Top Stories section in a different location for specific variants.
These changes will only show on specific AMP assets for news and sport services for this PR.

No other services should be affected. These changes only affect specific assets initially to ensure the experimentation works as expected on pages served by the AMP cache and that performance isn't somehow significantly impacted.

This PR also does not have any analytics work completed yet (to be done in the next PR), hence it does not start the experiment or put it 'live' per se.

Code changes

Created <AmpExperiment/> component (similar to other existing AMP components e.g. AmpIframe)
- Accepts a config containing experiment data so this component is not tied to this experiment and can be used for other AMP experiments in the future if needed.
Created /experimentTopStories folder in pages/ArticlePage that contains most of the code added specifically for this experiment.
- Added a config to be used in <AmpExperiment/> for the experiment.
- Created ExperimentTopStories that wraps a <TopStoriesSection/> component with experiment specific styling (to show/hide via <AmpExperiment/>).
- Added helper function insertExperimentTopStories() that inserts a experimentTopStories block into the blocks array in the halfway position of the array (this is the first variant of the experiment).
  - There were discussions as to where this logic should be done. It was concluded that it can be done here for the experiment, but should there be a full implementation of this, it would of course be better done with other transformations in the simorgh-bff FABL module.
- Added helper function enableExperimentTopStories() to check if the conditions to run insertExperimentTopStories() are fulfilled.
Refactored ArticlePage.tsx and ArticlePage.styles.ts to handle changes added specifically for the experiment.
- Added isAmp and pathname to list of props retrieved from the request context.
- Pulled topStoriesContent out early to be passed into the <ExperimentTopStories/> component in componentsToRender. This does not replace any existing logic with the Top Stories section.
- Replaced blocks array used in articleBlocks (where a visually hidden heading block is added to blocks array) with blocksWithExperimentTopStories. This will be the same as the blocks array if functions to insert an experimentTopStories block are not run, so this should not impact anything besides the specific news and sport assets.
- Separated topStoriesAndFeaturesSection css into topStoriesSection and featuresSection. This is so that styles that show/hide each Top Stories section will not affect anything else in the secondary column.
  - Added css for experimentTopStories blocks so that they only appear on one column breakpoints.
  - Added css to topStoriesSection so that it is hidden for specific variants on one column breakpoints.

Testing

Ensure the following assets behave as expected and does not break AMP validation.
Override links below with specific variant are formatted as such:
/service/articles/xyz.amp?renderer_env=env#amp-x-experimentName=variant
When checking other pages with services not affected by this PR, ensure that <amp-experiment/> is not present in the DOM. It should be present for the assets below:

Control (no changes to existing behaviour):

Variant name in code: control

Variant 1 (show Top Stories section halfway into the article content instead of after it):

Variant name in code: show_at_halfway

Helpful Links

AMP experiment documentation

iainrj

Looking really nice! I've only commented on things I can speak to and nothing really around the AMP implementation which I'll leave to someone who understands that in Simorgh more deeply.

src/app/components/AmpExperiment/index.tsx

iainrj · 2024-10-15T11:46:28Z

src/app/pages/ArticlePage/ArticlePage.tsx

+  let blocksWithExperimentTopStories = blocks;
+  const topStoriesContent = pageData?.secondaryColumn?.topStories;
+  const shouldEnableExperimentTopStories =
+    enableExperimentTopStories({
+      isAmp,
+      service,
+      pathname,
+    }) && topStoriesContent;
+
+  if (shouldEnableExperimentTopStories) {
+    blocksWithExperimentTopStories = insertExperimentTopStories({
+      blocks,
+      topStoriesContent,
+    });
+  }


Suggestion (non-blocking) Move this into the experiment top stories helper you've already created and just return either the blocks unchanged or the blocks with the top stories moved (as well as other things you might need)? This would hide the implementation detail of this experiment from anyone who is just parsing the article page code.

I originally opted to keep this here since shouldEnableExperimentTopStories is needed to conditionally render <AmpExperiment/> on the correct pages too, but after fiddling with it I think your suggestion is cleaner. Refactored so that it's just one export from the helpers file that returns blocks and shouldEnableExperimentTopStories.

iainrj · 2024-10-15T11:47:34Z

src/app/pages/ArticlePage/experimentTopStories/helpers.tsx

+  const newsTestAsset = 'c6v11qzyv8po';
+  const newsAsset = 'cz7xywn940ro';
+  const sportAsset = 'cpgw0xjmpd3o';


Question I assume this is for testing only?

For this initial PR yes. I thought it would be a good idea to just keep these restricted to one asset from each service we're impacting just to be extra safe. Will remove this in the following PR once we're happy with it all.

billsheikh1

Looks good. Leaving general comment but not on the AMP specific stuff

src/app/pages/ArticlePage/ArticlePage.tsx

src/app/components/AmpExperiment/index.tsx

src/app/pages/ArticlePage/experimentTopStories/helpers.test.tsx

src/app/pages/ArticlePage/experimentTopStories/helpers.tsx

src/app/pages/ArticlePage/index.test.tsx

src/app/pages/ArticlePage/experimentTopStories/helpers.tsx

Co-authored-by: Karina Thomas <[email protected]>

Isabella-Mitchell · 2024-10-16T09:17:32Z

src/app/pages/ArticlePage/ArticlePage.tsx

@@ -183,8 +203,8 @@ const ArticlePage = ({ pageData }: { pageData: Article }) => {
  };

  const articleBlocks = startsWithHeading
-    ? blocks
-    : [visuallyHiddenBlock, ...blocks];
+    ? transformedBlocks


I'm a bit confused on what's happening here. Does this always add in the experiment?

I think it might be. This was flagged by a TS error I get on line 102

Right operand of ?? is unreachable because the left operand is never nullish.

If I add something in like the below on line 205 then the unit tests pass

const blocksToUse = shouldEnableExperimentTopStories ? transformedBlocks : blocks;

I think the function now returns either the blocks unchanged or the experimentally changes to the blocks. This should probably be made more clear by updating the name of the function to de-couple it from experimentation. i.e getExperimentTopStoriesBlocks should really be getTopStoriesBlocks

Sorry that was just me being careless. You're right about the fix, I moved around a few things in the last refactor and completely left the check out. Re-added, should make sense now. Also missed some more renaming, but should be good now!

@iainrj I've kept the name getExperimentTopStoriesBlocks since it doesn't get the data for the original TopStoriesSection in the secondary column and because the top stories blocks in the article blocks array have type: experimentTopStories.

Nice one, thanks :)

Isabella-Mitchell

Have checked test articles and can see this rendering as expected on smaller breakpoints.

I appreciate this is for PS users - do we need our UX/ Accessibility peers to check this? Or is that being handled by your team? 🙂

hotinglok · 2024-10-16T11:06:29Z

@Isabella-Mitchell I discussed this with @eagerterrier prior to starting this work and it was agreed that since it's just an experiment and there aren't any actual changes to the Top Stories section besides the position of it, we should be ok without full UX/accessibility checks.

Chris-Kay · 2024-10-16T12:18:25Z

src/app/pages/ArticlePage/experimentTopStories/helpers.test.tsx

+      ${'only service is undefined'}                 | ${true}  | ${'c6v11qzyv8po'} | ${undefined}
+      ${'only service is defined and valid'}         | ${false} | ${undefined}      | ${'news'}
+      ${'all props defined but service is invalid'}  | ${true}  | ${'c6v11qzyv8po'} | ${'igbo'}
+    `(


Impressive I like that

src/app/components/AmpExperiment/index.tsx

Co-authored-by: Michelle <[email protected]>

hotinglok · 2024-10-17T12:08:40Z

src/app/pages/ArticlePage/experimentTopStories/helpers.tsx

-      })
-    : blocks;
+  const transformedBlocks =
+    shouldEnableExperimentTopStories && blocks.length > 2


Added the blocks.length > 2 condition to account for:

The current formula for calculating the halfway position placing the experiment top stories block before the original block if there is only one block in the array.

The experiment top stories block being placed before any content if there were only two blocks (e.g. headline block + image block).

hotinglok and others added 12 commits October 9, 2024 11:33

Create AmpExperiment component

9a2290e

Update tests

cdd54dd

Implement front-end changes

bdd3edd

Refactor various logic

a215780

Fix urn bug

a4b4188

Refactor various logic, add tests

b11f47a

Refactor block insertion logic, add article page tests

739314b

Update block insertion tests

db2b3ff

Merge branch 'latest' into nasaa-create-amp-experiment

8f709d0

Fix urn retrieval function

bcc34e5

Minor cleanup

6aae312

Minor cleanup

3be6ae5

hotinglok marked this pull request as ready for review October 15, 2024 11:39

iainrj reviewed Oct 15, 2024

View reviewed changes

billsheikh1 reviewed Oct 15, 2024

View reviewed changes

src/app/pages/ArticlePage/ArticlePage.tsx Outdated Show resolved Hide resolved

karinathomasbbc reviewed Oct 15, 2024

View reviewed changes

Isabella-Mitchell reviewed Oct 15, 2024

View reviewed changes

src/app/pages/ArticlePage/experimentTopStories/helpers.tsx Outdated Show resolved Hide resolved

hotinglok and others added 7 commits October 15, 2024 15:29

Rename, move, update various files and variables

cfe5254

Apply suggestions from code review

3e46a76

Co-authored-by: Karina Thomas <[email protected]>

Use id instead of pathname for urn

1725aa5

fix confflicts

1c4c59a

fix tests

4689617

Linting

cf7a1f1

Refactor helper functions

68d5932

hotinglok requested review from karinathomasbbc and Isabella-Mitchell October 15, 2024 17:24

Fix test

1674d2e

Isabella-Mitchell reviewed Oct 16, 2024

View reviewed changes

hotinglok added 2 commits October 16, 2024 10:27

Fix accidentally removed check in getExperimentTopStoriesBlocks()

43f903c

Linting

92a84f8

Isabella-Mitchell approved these changes Oct 16, 2024

View reviewed changes

hotinglok mentioned this pull request Oct 16, 2024

Add amp-experimentation to simorgh #11939

Closed

karinathomasbbc approved these changes Oct 16, 2024

View reviewed changes

Chris-Kay reviewed Oct 16, 2024

View reviewed changes

Chris-Kay approved these changes Oct 16, 2024

View reviewed changes

michellebbc reviewed Oct 16, 2024

View reviewed changes

src/app/components/AmpExperiment/index.tsx Outdated Show resolved Hide resolved

Update src/app/components/AmpExperiment/index.tsx

5bacd6f

Co-authored-by: Michelle <[email protected]>

michellebbc approved these changes Oct 17, 2024

View reviewed changes

hotinglok added 2 commits October 17, 2024 10:37

Rename getExperimentTopStoriesBlocks to getExperimentTopStories

a031332

Add article length condition to getExperimentTopStories()

31fbb08

hotinglok commented Oct 17, 2024

View reviewed changes

hotinglok and others added 2 commits October 17, 2024 13:36

Update helpers test

42ad63a

Merge branch 'latest' into nasaa-create-amp-experiment

bf3c63e

HarveyPeachey merged commit e9c6b27 into latest Oct 21, 2024
11 checks passed

HarveyPeachey deleted the nasaa-create-amp-experiment branch October 21, 2024 09:45

HarveyPeachey mentioned this pull request Oct 21, 2024

Revert "[NASAA-203/204] - Create AmpExperiment component, add top stories experiment to PS articles" #12066

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NASAA-203/204] - Create AmpExperiment component, add top stories experiment to PS articles #12048

[NASAA-203/204] - Create AmpExperiment component, add top stories experiment to PS articles #12048

hotinglok commented Oct 14, 2024 •

edited

Loading

iainrj left a comment

iainrj Oct 15, 2024

hotinglok Oct 15, 2024

iainrj Oct 15, 2024

hotinglok Oct 15, 2024

billsheikh1 left a comment

Isabella-Mitchell Oct 16, 2024

Isabella-Mitchell Oct 16, 2024

iainrj Oct 16, 2024

hotinglok Oct 16, 2024

hotinglok Oct 16, 2024

Isabella-Mitchell Oct 16, 2024

Isabella-Mitchell left a comment

hotinglok commented Oct 16, 2024

Chris-Kay Oct 16, 2024

hotinglok Oct 17, 2024

[NASAA-203/204] - Create AmpExperiment component, add top stories experiment to PS articles #12048

[NASAA-203/204] - Create AmpExperiment component, add top stories experiment to PS articles #12048

Conversation

hotinglok commented Oct 14, 2024 • edited Loading

Overall changes

Code changes

Testing

Control (no changes to existing behaviour):

Variant 1 (show Top Stories section halfway into the article content instead of after it):

Helpful Links

iainrj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billsheikh1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Isabella-Mitchell left a comment

Choose a reason for hiding this comment

hotinglok commented Oct 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hotinglok commented Oct 14, 2024 •

edited

Loading