Skip to content

Stop showing the pointer cursor when hovering over Cinemagraphs in articles #15163

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

Closed
simonbyford wants to merge 2 commits into from
+37 −15
Closed

Stop showing the pointer cursor when hovering over Cinemagraphs in articles #15163

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cbe96ae
letterboxed -> !isInArticle
simonbyford Jan 20, 2026
5ea9fbc
Don't show pointer when Cinemagraph is in article
simonbyford Jan 20, 2026
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
1 change: 0 additions & 1 deletion dotcom-rendering/src/components/Card/Card.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1081,7 +1081,6 @@ export const Card = ({
media.mainMedia.subtitleSource
}
subtitleSize={subtitleSize}
letterboxed={true}
/>
</Island>
)}
Expand Down
17 changes: 11 additions & 6 deletions dotcom-rendering/src/components/SelfHostedVideo.importable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,20 @@ const videoContainerStyles = (

const figureStyles = (
aspectRatio: number,
letterboxed: boolean,
isInArticle: boolean,
containerAspectRatio?: number,
isFeatureCard?: boolean,
) => css`
position: relative;
aspect-ratio: ${aspectRatio};
height: 100%;

${letterboxed &&
/**
* Videos on cards (i.e. not in articles) should not exceed the viewport
* height. Instead, they should be scaled and grey bars should be shown on
* either side.
*/
${!isInArticle &&
css`
max-height: 100vh;
max-height: 100svh;
Expand Down Expand Up @@ -166,7 +171,7 @@ type Props = {
linkTo: string;
subtitleSource?: string;
subtitleSize: SubtitleSize;
letterboxed?: boolean;
isInArticle?: boolean;
Copy link
Contributor

@domlander domlander Jan 22, 2026

Choose a reason for hiding this comment

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

Same as https://github.com/guardian/dotcom-rendering/pull/15163/changes#r2716362592

isFeatureCard?: boolean;
};

Expand All @@ -187,7 +192,7 @@ export const SelfHostedVideo = ({
linkTo,
subtitleSource,
subtitleSize,
letterboxed = false,
isInArticle = false,
isFeatureCard = false,
}: Props) => {
const adapted = useShouldAdapt();
Expand Down Expand Up @@ -720,7 +725,7 @@ export const SelfHostedVideo = ({
ref={setNode}
css={figureStyles(
aspectRatio,
letterboxed,
isInArticle,
containerAspectRatio,
isFeatureCard,
)}
Expand Down Expand Up @@ -756,7 +761,7 @@ export const SelfHostedVideo = ({
subtitleSource={subtitleSource}
subtitleSize={subtitleSize}
activeCue={activeCue}
letterboxed={letterboxed}
isInArticle={isInArticle}
isFeatureCard={isFeatureCard}
/>
</figure>
Expand Down
1 change: 1 addition & 0 deletions dotcom-rendering/src/components/SelfHostedVideoInArticle.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ export const SelfHostedVideoInArticle = ({
videoStyle={videoStyle}
uniqueId={element.id}
width={firstVideoAsset?.dimensions?.width ?? 500}
isInArticle={true}
/>
</Island>
{!!caption && (
Expand Down
33 changes: 25 additions & 8 deletions dotcom-rendering/src/components/SelfHostedVideoPlayer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,19 +25,30 @@ export type SubtitleSize = 'small' | 'medium' | 'large';

const videoStyles = (
aspectRatio: number,
letterboxed: boolean,
isInArticle: boolean,
isFeatureCard: boolean,
isCinemagraph: boolean,
) => css`
position: relative;
display: block;
height: auto;
width: 100%;
${letterboxed &&
/**
* Videos on cards (i.e. not in articles) should not exceed the viewport
* height. Instead, they should be scaled and grey bars should be shown on
* either side.
*/
${!isInArticle &&
Copy link
Contributor

@domlander domlander Jan 22, 2026

Choose a reason for hiding this comment

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

No longer needed following #15119

css`
max-height: 100vh;
max-height: 100svh;
`}
cursor: pointer;
/**
* Cinemagraphs should only show a pointer cursor when on a card (where clicking
* navigates to the article). In articles, they should not show a pointer cursor
* as clicking does nothing.
*/
cursor: ${isCinemagraph && isInArticle ? 'default' : 'pointer'};

/* Prevents CLS by letting the browser know the space the video will take up. */
aspect-ratio: ${aspectRatio};
Expand Down Expand Up @@ -139,7 +150,7 @@ type Props = {
subtitleSize?: SubtitleSize;
/* used in custom subtitle overlays */
activeCue?: ActiveCue | null;
letterboxed: boolean;
isInArticle: boolean;
Copy link
Contributor

@domlander domlander Jan 22, 2026

Choose a reason for hiding this comment

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

Could we use a prop that's more descriptive of the change it will make within this component? This should now be possible given that the max-height restrictions have been removed (Again in #15119). Something like isClickable or isInteractive?

isFeatureCard: boolean;
};

Expand Down Expand Up @@ -182,17 +193,18 @@ export const SelfHostedVideoPlayer = forwardRef(
subtitleSource,
subtitleSize,
activeCue,
letterboxed,
isInArticle,
isFeatureCard,
}: Props,
ref: React.ForwardedRef<HTMLVideoElement>,
) => {
const videoId = `video-${uniqueId}`;
const isCinemagraph = videoStyle === 'Cinemagraph';
const showSubtitles =
videoStyle !== 'Cinemagraph' && !!subtitleSource && !!subtitleSize;
!isCinemagraph && !!subtitleSource && !!subtitleSize;

const showControls =
videoStyle !== 'Cinemagraph' &&
!isCinemagraph &&
ref &&
'current' in ref &&
ref.current &&
Expand All @@ -210,7 +222,12 @@ export const SelfHostedVideoPlayer = forwardRef(
<video
id={videoId}
css={[
videoStyles(aspectRatio, letterboxed, isFeatureCard),
videoStyles(
aspectRatio,
isInArticle,
isFeatureCard,
isCinemagraph,
),
showSubtitles && subtitleStyles(subtitleSize),
]}
crossOrigin="anonymous"
Expand Down
Loading