Skip to content

Commit

Permalink
Merge pull request #12987 from guardian/optimize-useIsAndroid-hook
Browse files Browse the repository at this point in the history
Remove useIsAndroid Hook and add Support for Horizontal Scrolling
  • Loading branch information
marjisound authored Jan 27, 2025
2 parents b4ed63f + 123a5a7 commit b21e00f
Show file tree
Hide file tree
Showing 21 changed files with 132 additions and 55 deletions.
22 changes: 19 additions & 3 deletions dotcom-rendering/src/components/Carousel.importable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,11 @@ import {
import libDebounce from 'lodash.debounce';
import { useEffect, useRef, useState } from 'react';
import { ArticleDesign, type ArticleFormat } from '../lib/articleFormat';
import { getInteractionClient } from '../lib/bridgetApi';
import { formatAttrString } from '../lib/formatAttrString';
import { getSourceImageUrl } from '../lib/getSourceImageUrl_temp_fix';
import { getZIndex } from '../lib/getZIndex';
import { useIsAndroid } from '../lib/useIsAndroid';
import { useIsHorizontalScrollingSupported } from '../lib/useIsHorizontalScrollingSupported';
import { palette as themePalette } from '../palette';
import type { Branding } from '../types/branding';
import type { StarRating } from '../types/content';
Expand All @@ -24,6 +25,7 @@ import type {
import type { LeftColSize } from '../types/layout';
import type { MainMedia } from '../types/mainMedia';
import type { OnwardsSource } from '../types/onwards';
import type { RenderingTarget } from '../types/renderingTarget';
import type { TrailType } from '../types/trails';
import { Card } from './Card/Card';
import { LI } from './Card/components/LI';
Expand All @@ -42,6 +44,7 @@ type Props = {
leftColSize: LeftColSize;
discussionApiUrl: string;
absoluteServerTimes: boolean;
renderingTarget: RenderingTarget;
};

type ArticleProps = Props & {
Expand Down Expand Up @@ -792,13 +795,16 @@ export const Carousel = ({
discussionApiUrl,
isOnwardContent = true,
absoluteServerTimes,
renderingTarget,
...props
}: ArticleProps | FrontProps) => {
const carouselRef = useRef<HTMLUListElement>(null);

const [index, setIndex] = useState(0);
const [maxIndex, setMaxIndex] = useState(0);
const isAndroid = useIsAndroid();
const isHorizontalScrollingSupported = useIsHorizontalScrollingSupported();

const isApps = renderingTarget === 'Apps';

const arrowName = 'carousel-small-arrow';

Expand Down Expand Up @@ -907,7 +913,15 @@ export const Carousel = ({
// when index changes and compare it against the prior maxIndex only.
useEffect(() => setMaxIndex((m) => Math.max(index, m)), [index]);

if (isAndroid) {
const onTouchStart = async () => {
await getInteractionClient().disableArticleSwipe(true);
};

const onTouchEnd = async () => {
await getInteractionClient().disableArticleSwipe(false);
};

if (!isHorizontalScrollingSupported) {
return null;
}

Expand Down Expand Up @@ -974,6 +988,8 @@ export const Carousel = ({
ref={carouselRef}
data-component={`carousel-small | maxIndex-${maxIndex}`}
data-heatphan-type="carousel"
onTouchStart={isApps ? onTouchStart : undefined}
onTouchEnd={isApps ? onTouchEnd : undefined}
>
{trails.map((trail, i) => {
const {
Expand Down
7 changes: 7 additions & 0 deletions dotcom-rendering/src/components/Carousel.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,7 @@ export const Headlines: StoryObj = ({ format }: StoryProps) => {
leftColSize="compact"
discussionApiUrl={discussionApiUrl}
absoluteServerTimes={true}
renderingTarget="Web"
/>
</Section>
);
Expand All @@ -295,6 +296,7 @@ export const SingleItemCarousel = () => {
leftColSize="compact"
discussionApiUrl={discussionApiUrl}
absoluteServerTimes={true}
renderingTarget="Web"
/>
</Section>
);
Expand Down Expand Up @@ -349,6 +351,7 @@ export const SingleOpinionCarousel = () => {
leftColSize="compact"
discussionApiUrl={discussionApiUrl}
absoluteServerTimes={true}
renderingTarget="Web"
/>
</Section>
);
Expand Down Expand Up @@ -379,6 +382,7 @@ export const Immersive = () => {
leftColSize="compact"
discussionApiUrl={discussionApiUrl}
absoluteServerTimes={true}
renderingTarget="Web"
/>
</Section>
</>
Expand Down Expand Up @@ -424,6 +428,7 @@ export const SpecialReportAlt = () => {
leftColSize="compact"
discussionApiUrl={discussionApiUrl}
absoluteServerTimes={true}
renderingTarget="Web"
/>
</Section>
);
Expand Down Expand Up @@ -639,6 +644,7 @@ export const AllCards = () => {
discussionApiUrl={discussionApiUrl}
format={defaultFormat}
absoluteServerTimes={true}
renderingTarget="Web"
/>
</Section>
);
Expand All @@ -663,6 +669,7 @@ export const FrontCarousel = () => (
discussionApiUrl={discussionApiUrl}
palette="PodcastPalette"
absoluteServerTimes={true}
renderingTarget="Web"
/>
</Section>
</>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { useApi } from '../lib/useApi';
import { addDiscussionIds } from '../lib/useCommentCount';
import { palette } from '../palette';
import type { OnwardsSource } from '../types/onwards';
import type { RenderingTarget } from '../types/renderingTarget';
import type { FETrailType, TrailType } from '../types/trails';
import { Carousel } from './Carousel.importable';
import { Placeholder } from './Placeholder';
Expand All @@ -17,6 +18,7 @@ type Props = {
format: ArticleFormat;
discussionApiUrl: string;
absoluteServerTimes: boolean;
renderingTarget: RenderingTarget;
};

type OnwardsResponse = {
Expand All @@ -37,6 +39,7 @@ export const FetchOnwardsData = ({
format,
discussionApiUrl,
absoluteServerTimes,
renderingTarget,
}: Props) => {
const { data, error } = useApi<OnwardsResponse>(url);

Expand Down Expand Up @@ -85,6 +88,7 @@ export const FetchOnwardsData = ({
}
discussionApiUrl={discussionApiUrl}
absoluteServerTimes={absoluteServerTimes}
renderingTarget={renderingTarget}
/>
</div>
);
Expand Down
1 change: 1 addition & 0 deletions dotcom-rendering/src/components/Island.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ describe('Island: server-side rendering', () => {
shortUrlId=""
discussionApiUrl=""
absoluteServerTimes={true}
renderingTarget="Web"
/>
</WithConfig>,
),
Expand Down
12 changes: 9 additions & 3 deletions dotcom-rendering/src/components/OnwardsUpper.importable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@ import {
Pillar,
} from '../lib/articleFormat';
import type { EditionId } from '../lib/edition';
import { useIsAndroid } from '../lib/useIsAndroid';
import { useIsHorizontalScrollingSupported } from '../lib/useIsHorizontalScrollingSupported';
import { palette } from '../palette';
import type { OnwardsSource } from '../types/onwards';
import type { RenderingTarget } from '../types/renderingTarget';
import type { TagType } from '../types/tag';
import { FetchOnwardsData } from './FetchOnwardsData.importable';
import { Section } from './Section';
Expand Down Expand Up @@ -179,6 +180,7 @@ type Props = {
shortUrlId: string;
discussionApiUrl: string;
absoluteServerTimes: boolean;
renderingTarget: RenderingTarget;
};

/**
Expand Down Expand Up @@ -213,9 +215,11 @@ export const OnwardsUpper = ({
shortUrlId,
discussionApiUrl,
absoluteServerTimes,
renderingTarget,
}: Props) => {
const isAndroid = useIsAndroid();
if (isAndroid) return null;
const isHorizontalScrollingSupported = useIsHorizontalScrollingSupported();

if (!isHorizontalScrollingSupported) return null;

// Related content can be a collection of articles based on
// two things, 1: A popular tag, or 2: A generic text match
Expand Down Expand Up @@ -309,6 +313,7 @@ export const OnwardsUpper = ({
format={format}
discussionApiUrl={discussionApiUrl}
absoluteServerTimes={absoluteServerTimes}
renderingTarget={renderingTarget}
/>
</Section>
)}
Expand All @@ -324,6 +329,7 @@ export const OnwardsUpper = ({
format={format}
discussionApiUrl={discussionApiUrl}
absoluteServerTimes={absoluteServerTimes}
renderingTarget={renderingTarget}
/>
</Section>
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import { pickMessage } from '../lib/messagePicker';
import { useIsSignedIn } from '../lib/useAuthStatus';
import { useBraze } from '../lib/useBraze';
import { useCountryCode } from '../lib/useCountryCode';
import { useIsAndroid } from '../lib/useIsAndroid';
import { useSignInGateWillShow } from '../lib/useSignInGateWillShow';
import type { TagType } from '../types/tag';
import { useConfig } from './ConfigContext';
Expand Down Expand Up @@ -101,7 +100,6 @@ const buildRRBannerConfigWith = ({
tags,
contributionsServiceUrl,
idApiUrl,
isAndroidWebview,
}: {
isSignedIn: boolean;
countryCode: CountryCode;
Expand All @@ -117,7 +115,6 @@ const buildRRBannerConfigWith = ({
tags: TagType[];
contributionsServiceUrl: string;
idApiUrl: string;
isAndroidWebview: boolean;
}): CandidateConfig<BannerProps> => {
return {
candidate: {
Expand Down Expand Up @@ -152,7 +149,6 @@ const buildRRBannerConfigWith = ({
idApiUrl,
signInGateWillShow,
asyncArticleCounts,
isAndroidWebview,
}),
show:
({ meta, module, fetchEmail }: BannerProps) =>
Expand Down Expand Up @@ -246,7 +242,6 @@ export const StickyBottomBanner = ({
isPreview,
currentLocaleCode: countryCode,
});
const isAndroidWebview = !!useIsAndroid();

useEffect(() => {
setAsyncArticleCounts(getArticleCounts(pageId, tags, contentType));
Expand Down Expand Up @@ -283,7 +278,6 @@ export const StickyBottomBanner = ({
tags,
contributionsServiceUrl,
idApiUrl,
isAndroidWebview,
});
const brazeArticleContext: BrazeArticleContext = {
section: sectionId,
Expand Down Expand Up @@ -317,7 +311,6 @@ export const StickyBottomBanner = ({
contentType,
contributionsServiceUrl,
idApiUrl,
isAndroidWebview,
isMinuteArticle,
isPaidContent,
isPreview,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ type CanShowProps = BaseProps & {
idApiUrl: string;
signInGateWillShow: boolean;
asyncArticleCounts: Promise<ArticleCounts | undefined>;
isAndroidWebview: boolean;
};

type ReaderRevenueComponentType =
Expand Down Expand Up @@ -192,15 +191,9 @@ export const canShowRRBanner: CanShowFunctionType<BannerProps> = async ({
idApiUrl,
signInGateWillShow,
asyncArticleCounts,
isAndroidWebview,
}) => {
if (!remoteBannerConfig) return { show: false };

if (isAndroidWebview) {
// Do not show banners on Android app webview, due to buggy behaviour with the buttons
return { show: false };
}

if (
shouldHideReaderRevenue ||
isPaidContent ||
Expand Down
7 changes: 6 additions & 1 deletion dotcom-rendering/src/layouts/AudioLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import { parse } from '../lib/slot-machine-flags';
import type { NavType } from '../model/extract-nav';
import { palette as themePalette } from '../palette';
import type { ArticleDeprecated } from '../types/article';
import type { RenderingTarget } from '../types/renderingTarget';
import { BannerWrapper, Stuck } from './lib/stickiness';

const AudioGrid = ({ children }: { children: React.ReactNode }) => (
Expand Down Expand Up @@ -124,14 +125,16 @@ const maxWidth = css`
interface Props {
article: ArticleDeprecated;
format: ArticleFormat;
renderingTarget: RenderingTarget;
}

interface WebProps extends Props {
NAV: NavType;
renderingTarget: 'Web';
}

export const AudioLayout = (props: WebProps) => {
const { article, format } = props;
const { article, format, renderingTarget } = props;
const audioData = getAudioData(article.mainMediaElements);

const {
Expand Down Expand Up @@ -495,6 +498,7 @@ export const AudioLayout = (props: WebProps) => {
article.config.discussionApiUrl
}
absoluteServerTimes={absoluteServerTimes}
renderingTarget={renderingTarget}
/>
</Island>
</Section>
Expand All @@ -518,6 +522,7 @@ export const AudioLayout = (props: WebProps) => {
shortUrlId={article.config.shortUrlId}
discussionApiUrl={article.config.discussionApiUrl}
absoluteServerTimes={absoluteServerTimes}
renderingTarget={renderingTarget}
/>
</Island>
{showComments && (
Expand Down
2 changes: 2 additions & 0 deletions dotcom-rendering/src/layouts/CommentLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -737,6 +737,7 @@ export const CommentLayout = (props: WebProps | AppsProps) => {
article.config.discussionApiUrl
}
absoluteServerTimes={absoluteServerTimes}
renderingTarget={renderingTarget}
/>
</Island>
</Section>
Expand All @@ -760,6 +761,7 @@ export const CommentLayout = (props: WebProps | AppsProps) => {
shortUrlId={article.config.shortUrlId}
discussionApiUrl={article.config.discussionApiUrl}
absoluteServerTimes={absoluteServerTimes}
renderingTarget={renderingTarget}
/>
</Island>

Expand Down
2 changes: 2 additions & 0 deletions dotcom-rendering/src/layouts/DecideLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ const DecideLayoutWeb = ({
article={article}
NAV={NAV}
format={format}
renderingTarget={renderingTarget}
/>
);
case ArticleDesign.Audio:
Expand All @@ -271,6 +272,7 @@ const DecideLayoutWeb = ({
article={article}
format={format}
NAV={NAV}
renderingTarget={renderingTarget}
/>
);
default:
Expand Down
6 changes: 6 additions & 0 deletions dotcom-rendering/src/layouts/FrontLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
} from '@guardian/source/foundations';
import { AdSlot } from '../components/AdSlot.web';
import { Carousel } from '../components/Carousel.importable';
import { useConfig } from '../components/ConfigContext';
import { ContainerOverrides } from '../components/ContainerOverrides';
import { CPScottHeader } from '../components/CPScottHeader';
import { DecideContainer } from '../components/DecideContainer';
Expand Down Expand Up @@ -114,6 +115,8 @@ export const FrontLayout = ({ front, NAV }: Props) => {
editionId,
} = front;

const { renderingTarget } = useConfig();

const renderAds = canRenderAds(front);

const hasPageSkin = renderAds && hasPageSkinConfig;
Expand Down Expand Up @@ -618,6 +621,9 @@ export const FrontLayout = ({ front, NAV }: Props) => {
absoluteServerTimes={
absoluteServerTimes
}
renderingTarget={
renderingTarget
}
/>
</Island>
</Section>
Expand Down
Loading

0 comments on commit b21e00f

Please sign in to comment.