Skip to content

MostViewedFooter as an island #3942

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.

Sign up for GitHub

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

Merged
oliverlloyd merged 8 commits into from
Feb 10, 2022
Merged

MostViewedFooter as an island #3942

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6fc379e
feat: `MostViewedFooter` as an island
oliverlloyd Feb 8, 2022
c1fe828
refactor: Move MostViewedFotter files into the components root
oliverlloyd Feb 8, 2022
19ec8d1
refactor: Created `MostViewedFooterLayout`
oliverlloyd Feb 9, 2022
68b839f
Merge branch 'main' into oliver/mostviewedfooter-island
oliverlloyd Feb 9, 2022
7019d31
Merge branch 'main' into oliver/mostviewedfooter-island
oliverlloyd Feb 9, 2022
8747f67
feat: Added WithABProvider to MostViewedFooter
oliverlloyd Feb 9, 2022
6e1068c
fix: Imports
oliverlloyd Feb 9, 2022
082ec45
refactor: `SecondTierItem` > `MostViewedFooterSecondTierItem`
oliverlloyd Feb 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions dotcom-rendering/src/web/components/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import {
} from '@guardian/support-dotcom-components';
import { WeeklyArticleHistory } from '@guardian/support-dotcom-components/dist/dotcom/src/types';
import { ShareCount } from './ShareCount';
import { MostViewedFooter } from './MostViewed/MostViewedFooter/MostViewedFooter';
import { ReaderRevenueLinks } from './ReaderRevenueLinks';
import { SlotBodyEnd } from './SlotBodyEnd/SlotBodyEnd';
import { ContributionSlot } from './ContributionSlot';
Expand Down Expand Up @@ -399,13 +398,6 @@ export const App = ({ CAPI }: Props) => {
pageViewId={pageViewId}
/>
</Portal>
<Portal rootId="most-viewed-footer">
<MostViewedFooter
format={format}
sectionName={CAPI.sectionName}
ajaxUrl={CAPI.config.ajaxUrl}
/>
</Portal>
<Portal rootId="bottom-banner">
<StickyBottomBanner
brazeMessages={brazeMessages}
Expand Down
0 ...components/MostViewed/MostViewed.mocks.ts → ...ng/src/web/components/MostViewed.mocks.ts
View file
Open in desktop
File renamed without changes.
34 changes: 34 additions & 0 deletions dotcom-rendering/src/web/components/MostViewedFooter.importable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { MostViewedFooterLayout } from './MostViewedFooterLayout';
import { WithABProvider } from './WithABProvider';

type Props = {
sectionName?: string;
format: ArticleFormat;
ajaxUrl: string;
switches: Switches;
pageIsSensitive: boolean;
isDev?: boolean;
};

export const MostViewedFooter = ({
sectionName,
format,
ajaxUrl,
switches,
pageIsSensitive,
isDev,
}: Props) => {
return (
<WithABProvider
abTestSwitches={switches}
pageIsSensitive={pageIsSensitive}
isDev={!!isDev}
>
<MostViewedFooterLayout
sectionName={sectionName}
format={format}
ajaxUrl={ajaxUrl}
/>
</WithABProvider>

@jamesgorrie jamesgorrie Feb 9, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this reason we need WithABProvider here because this is being loaded in dynamically and so doesn't inherit the BootReact WithABProvider?

@oliverlloyd oliverlloyd Feb 9, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly. We're no longer in the same context (the same stack) as BootReact so we need to set our own AB provider

@jamesgorrie jamesgorrie Feb 10, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this a pattern we will continue to see then and could we look at adding this to the islands code? e.g. All islands have the same as the Boot context?

(another PR / discussion maybe)

@oliverlloyd oliverlloyd Feb 10, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I considered this approach (I'll update the PR description to include this) but it turned out to make the inclusion of this AB code dynamic (only when needed/wanted) I would have needed to change the code both in the Island server side component and also the doHydration client side code. It felt like adding more coupling to this for what is a secondary concern wasn't the right thing to do.

The alternative to the dynamic coupling is to just always set it. Which is an option. I guess the discussion is around the trade off between optimisation and dev ex. I think this is a worthwhile discussion to have so I'll raise it 👍

@oliverlloyd oliverlloyd Feb 10, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Discussion: #3955

);
};
15 changes: 12 additions & 3 deletions ...ViewedFooter/MostViewedFooter.stories.tsx → ...b/components/MostViewedFooter.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ import fetchMock from 'fetch-mock';
import { ArticleDisplay, ArticleDesign, ArticlePillar } from '@guardian/libs';
import { ABProvider } from '@guardian/ab-react';

import { ElementContainer } from '../../ElementContainer';
import { ElementContainer } from './ElementContainer';
import {
responseWithTwoTabs,
responseWithOneTab,
responseWithMissingImage,
} from '../MostViewed.mocks';
} from './MostViewed.mocks';

import { MostViewedFooter } from './MostViewedFooter';
import { MostViewedFooter } from './MostViewedFooter.importable';

export default {
component: MostViewedFooter,
Expand Down Expand Up @@ -51,6 +51,9 @@ export const withTwoTabs = () => {
}}
sectionName="politics"
ajaxUrl="https://api.nextgen.guardianapps.co.uk"
switches={{}}
pageIsSensitive={false}
isDev={false}
Comment on lines +54 to +56

@oliverlloyd oliverlloyd Feb 9, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I''m passing these in to be used by WithABProvider

/>
</ElementContainer>
</AbProvider>
Expand All @@ -74,6 +77,9 @@ export const withOneTabs = () => {
theme: ArticlePillar.News,
}}
ajaxUrl="https://api.nextgen.guardianapps.co.uk"
switches={{}}
pageIsSensitive={false}
isDev={false}
/>
</ElementContainer>
</AbProvider>
Expand All @@ -97,6 +103,9 @@ export const withNoMostSharedImage = () => {
theme: ArticlePillar.News,
}}
ajaxUrl="https://api.nextgen.guardianapps.co.uk"
switches={{}}
pageIsSensitive={false}
isDev={false}
/>
</ElementContainer>
</AbProvider>
Expand Down
8 changes: 4 additions & 4 deletions ...ostViewedFooter/MostViewedFooter.test.tsx → .../web/components/MostViewedFooter.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,16 @@ import { render, fireEvent } from '@testing-library/react';

import { ArticleDesign, ArticleDisplay, ArticlePillar } from '@guardian/libs';

import { useApi as useApi_ } from '../../../lib/useApi';
import { decidePalette } from '../../../lib/decidePalette';
import { useApi as useApi_ } from '../lib/useApi';

@oliverlloyd oliverlloyd Feb 9, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There's a bunch of these import changes which are merge conflict hangovers

import { decidePalette } from '../lib/decidePalette';

import { responseWithTwoTabs, responseWithOneTab } from '../MostViewed.mocks';
import { responseWithTwoTabs, responseWithOneTab } from './MostViewed.mocks';
import { MostViewedFooterData } from './MostViewedFooterData';

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const useApi: { [key: string]: any } = useApi_;

jest.mock('../../../lib/useApi', () => ({
jest.mock('../lib/useApi', () => ({
useApi: jest.fn(),
}));

Expand Down
12 changes: 6 additions & 6 deletions ...MostViewedFooter/MostViewedFooterData.tsx → ...c/web/components/MostViewedFooterData.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@ import { css } from '@emotion/react';

import { border, from, Breakpoint } from '@guardian/source-foundations';

import { useApi } from '../../../lib/useApi';
import { joinUrl } from '../../../../lib/joinUrl';
import { decideTrail } from '../../../lib/decideTrail';
import { useApi } from '../lib/useApi';
import { joinUrl } from '../../lib/joinUrl';
import { decideTrail } from '../lib/decideTrail';

import { MostViewedFooterGrid } from './MostViewedFooterGrid';
import { SecondTierItem } from './SecondTierItem';
import { MostViewedFooterSecondTierItem } from './MostViewedFooterSecondTierItem';

type Props = {
sectionName?: string;
Expand Down Expand Up @@ -81,15 +81,15 @@ export const MostViewedFooterData = ({
/>
<div css={[stackBelow('tablet'), secondTierStyles]}>
{'mostCommented' in data && (
<SecondTierItem
<MostViewedFooterSecondTierItem
trail={decideTrail(data.mostCommented)}
title="Most commented"
dataLinkName="comment | group-0 | card-@1" // To match Frontend
showRightBorder={true}
/>
)}
{'mostShared' in data && (
<SecondTierItem
<MostViewedFooterSecondTierItem
trail={decideTrail(data.mostShared)}
dataLinkName="news | group-0 | card-@1" // To match Frontend
title="Most shared"
Expand Down
0 ...MostViewedFooter/MostViewedFooterGrid.tsx → ...c/web/components/MostViewedFooterGrid.tsx
View file
Open in desktop
File renamed without changes.
6 changes: 3 additions & 3 deletions ...MostViewedFooter/MostViewedFooterItem.tsx → ...c/web/components/MostViewedFooterItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,10 @@ import {
headline,
until,
} from '@guardian/source-foundations';
import { BigNumber } from '../../BigNumber/BigNumber';
import { AgeWarning } from '../../AgeWarning';
import { BigNumber } from './BigNumber/BigNumber';
import { AgeWarning } from './AgeWarning';

import { LinkHeadline } from '../../LinkHeadline';
import { LinkHeadline } from './LinkHeadline';

const gridItem = (position: number) => css`
position: relative;
Expand Down
48 changes: 16 additions & 32 deletions ...wed/MostViewedFooter/MostViewedFooter.tsx → ...web/components/MostViewedFooterLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,13 @@
import React, { Suspense } from 'react';
import { css } from '@emotion/react';

import { text, headline, from, Breakpoint } from '@guardian/source-foundations';

import { useAB } from '@guardian/ab-react';
import { ArticleDesign } from '@guardian/libs';
import { initPerf } from '../../../browser/initPerf';
import { AdSlot, labelStyles } from '../../AdSlot';
import { Lazy } from '../../Lazy';

import { abTestTest } from '../../../experiments/tests/ab-test-test';
import { decidePalette } from '../../../lib/decidePalette';
import { Hide } from '../../Hide';
import { LeftColumn } from '../../LeftColumn';

const MostViewedFooterData = React.lazy(() => {
const { start, end } = initPerf('MostViewedFooterData');
start();
return import(
/* webpackChunkName: "MostViewedFooterData" */ './MostViewedFooterData'
).then((module) => {
end();
return { default: module.MostViewedFooterData };
});
});
import { Hide } from './Hide';
import { LeftColumn } from './LeftColumn';
import { MostViewedFooterData } from './MostViewedFooterData';
import { AdSlot, labelStyles } from './AdSlot';
import { abTestTest } from '../experiments/tests/ab-test-test';
import { decidePalette } from '../lib/decidePalette';

const stackBelow = (breakpoint: Breakpoint) => css`
display: flex;
Expand Down Expand Up @@ -82,7 +66,11 @@ interface Props {
ajaxUrl: string;
}

export const MostViewedFooter = ({ sectionName, format, ajaxUrl }: Props) => {
export const MostViewedFooterLayout = ({
sectionName,
format,
ajaxUrl,
}: Props) => {
// Example usage of AB Tests
// Used in the Cypress tests as smoke test of the AB tests framework integration
const ABTestAPI = useAB();
Expand Down Expand Up @@ -130,15 +118,11 @@ export const MostViewedFooter = ({ sectionName, format, ajaxUrl }: Props) => {
<Hide when="above" breakpoint="leftCol">
<h2 css={headingStyles}>Most popular</h2>
</Hide>
<Lazy margin={300}>
Comment thread
oliverlloyd marked this conversation as resolved.
<Suspense fallback={<></>}>
<MostViewedFooterData
sectionName={sectionName}
palette={palette}
ajaxUrl={ajaxUrl}
/>
</Suspense>
</Lazy>
<MostViewedFooterData
sectionName={sectionName}
palette={palette}
ajaxUrl={ajaxUrl}
/>
</div>
<div
css={css`
Expand Down
12 changes: 6 additions & 6 deletions ...iewed/MostViewedFooter/SecondTierItem.tsx → ...onents/MostViewedFooterSecondTierItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ import {
from,
} from '@guardian/source-foundations';

import { AgeWarning } from '../../AgeWarning';
import { Avatar } from '../../Avatar';
import { LinkHeadline } from '../../LinkHeadline';
import { Flex } from '../../Flex';
import { decidePalette } from '../../../lib/decidePalette';
import { AgeWarning } from './AgeWarning';
import { Avatar } from './Avatar';
import { LinkHeadline } from './LinkHeadline';
import { Flex } from './Flex';
import { decidePalette } from '../lib/decidePalette';

const itemStyles = (showRightBorder?: boolean) => css`
position: relative;
Expand Down Expand Up @@ -83,7 +83,7 @@ type Props = {
dataLinkName: string;
};

export const SecondTierItem = ({
export const MostViewedFooterSecondTierItem = ({
trail,
title,
showRightBorder,
Expand Down
2 changes: 1 addition & 1 deletion dotcom-rendering/src/web/components/MostViewedRight.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { LeftColumn } from './LeftColumn';
import { ArticleContainer } from './ArticleContainer';
import { ElementContainer } from './ElementContainer';

import { mockTab1 } from './MostViewed/MostViewed.mocks';
import { mockTab1 } from './MostViewed.mocks';
import { MostViewedRight } from './MostViewedRight';

export default {
Expand Down
2 changes: 1 addition & 1 deletion dotcom-rendering/src/web/components/MostViewedRight.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { render } from '@testing-library/react';

import { useApi as useApi_ } from '../lib/useApi';

import { mockTab1 } from './MostViewed/MostViewed.mocks';
import { mockTab1 } from './MostViewed.mocks';
import { MostViewedRight } from './MostViewedRight';

const response = { data: mockTab1 };
Expand Down
17 changes: 13 additions & 4 deletions dotcom-rendering/src/web/layouts/CommentLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import { Island } from '../components/Island';
import { MostViewedRightWrapper } from '../components/MostViewedRightWrapper.importable';
import { OnwardsUpper } from '../components/OnwardsUpper.importable';
import { OnwardsLower } from '../components/OnwardsLower.importable';
import { MostViewedFooter } from '../components/MostViewedFooter.importable';

const StandardGrid = ({
children,
Expand Down Expand Up @@ -686,10 +687,18 @@ export const CommentLayout = ({
)}

{!isPaidContent && (
<ElementContainer
sectionId="most-viewed-footer"
element="aside"
/>
<ElementContainer data-print-layout="hide" element="aside">
<Island clientOnly={true} deferUntil="visible">
<MostViewedFooter
format={format}
sectionName={CAPI.sectionName}
ajaxUrl={CAPI.config.ajaxUrl}
switches={CAPI.config.switches}
pageIsSensitive={CAPI.config.isSensitive}
isDev={CAPI.config.isDev}
/>
</Island>
</ElementContainer>
)}

<ElementContainer
Expand Down
17 changes: 13 additions & 4 deletions dotcom-rendering/src/web/layouts/ImmersiveLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import { ImmersiveHeader } from './headers/ImmersiveHeader';
import { Island } from '../components/Island';
import { OnwardsLower } from '../components/OnwardsLower.importable';
import { OnwardsUpper } from '../components/OnwardsUpper.importable';
import { MostViewedFooter } from '../components/MostViewedFooter.importable';

const ImmersiveGrid = ({ children }: { children: React.ReactNode }) => (
<div
Expand Down Expand Up @@ -508,10 +509,18 @@ export const ImmersiveLayout = ({
)}

{!isPaidContent && (
<ElementContainer
sectionId="most-viewed-footer"
element="aside"
/>
<ElementContainer data-print-layout="hide" element="aside">
<Island clientOnly={true} deferUntil="visible">
<MostViewedFooter
format={format}
sectionName={CAPI.sectionName}
ajaxUrl={CAPI.config.ajaxUrl}
switches={CAPI.config.switches}
pageIsSensitive={CAPI.config.isSensitive}
isDev={CAPI.config.isDev}
/>
</Island>
</ElementContainer>
)}

<ElementContainer
Expand Down
18 changes: 13 additions & 5 deletions dotcom-rendering/src/web/layouts/InteractiveLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ import {
import { Island } from '../components/Island';
import { OnwardsLower } from '../components/OnwardsLower.importable';
import { OnwardsUpper } from '../components/OnwardsUpper.importable';
import { MostViewedFooter } from '../components/MostViewedFooter.importable';

const InteractiveGrid = ({ children }: { children: React.ReactNode }) => (
<div
Expand Down Expand Up @@ -629,11 +630,18 @@ export const InteractiveLayout = ({ CAPI, NAV, format, palette }: Props) => {
)}

{!isPaidContent && (
<ElementContainer
data-print-layout="hide"
sectionId="most-viewed-footer"
element="aside"
/>
<ElementContainer data-print-layout="hide" element="aside">
<Island clientOnly={true} deferUntil="visible">
<MostViewedFooter
format={format}
sectionName={CAPI.sectionName}
ajaxUrl={CAPI.config.ajaxUrl}
switches={CAPI.config.switches}
pageIsSensitive={CAPI.config.isSensitive}
isDev={CAPI.config.isDev}
/>
</Island>
</ElementContainer>
)}

<ElementContainer
Expand Down
Loading