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.

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

refactor(Card): extract image #6066

Merged
merged 3 commits into from
Oct 3, 2022
Merged

refactor(Card): extract image #6066

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c615d83
refactor(Card): extract image
mxdvl Sep 27, 2022
b27392f
Update dotcom-rendering/src/web/components/Card/Card.tsx
mxdvl Sep 27, 2022
6f7655d
imagePosition and mobileImagePosition are non-optional
ioannakok Sep 29, 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
83 changes: 44 additions & 39 deletions dotcom-rendering/src/web/components/Card/Card.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,18 @@ const CommentFooter = ({
);
};

const getImage = ({
imageUrl,
avatarUrl,
}: {
imageUrl?: string;
avatarUrl?: string;
}): { type: CardImageType; src: string } | undefined => {
if (avatarUrl) return { type: 'avatar', src: avatarUrl };
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic looks to be wrong? The previous lines:

if (imageUrl && avatarUrl) {
	imageType = 'avatar';
}

So this should be

if (avatarUrl && imageUrl) return { type: 'avatar', src: avatarUrl };

That said I don't think I fully understand why this logic is the way it is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could not figure it out either, and this causes no visual difference, so tempted to simplify it like so.

If we want to capture a missing imageUrl and a set avatarUrl, we should have a story against it.

Copy link
Contributor

@ioannakok ioannakok Sep 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I agree the previous logic is a bit weird and I prefer the new version. I'll leave this up to the two of you. On my side, I'm happy with this PR so I have approved.

if (imageUrl) return { type: 'mainMedia', src: imageUrl };
return undefined;
};

export const Card = ({
linkTo,
format,
Expand Down Expand Up @@ -280,13 +292,10 @@ export const Card = ({
return <Snap snapData={snapData} />;
}

// Decide what type of image to show, main media, avatar or none
let imageType: CardImageType | undefined;
if (imageUrl && avatarUrl) {
imageType = 'avatar';
} else if (imageUrl) {
imageType = 'mainMedia';
}
const image = getImage({
imageUrl,
avatarUrl,
});

return (
<CardWrapper
Expand All @@ -301,41 +310,37 @@ export const Card = ({
dataLinkName={dataLinkName}
/>
<CardLayout
imagePosition={imageUrl !== undefined ? imagePosition : 'top'}
imagePositionOnMobile={
imageUrl !== undefined ? imagePositionOnMobile : 'top'
}
imagePosition={image ? imagePosition : 'top'}
imagePositionOnMobile={image ? imagePositionOnMobile : 'top'}
mxdvl marked this conversation as resolved.
Show resolved Hide resolved
minWidthInPixels={minWidthInPixels}
imageType={imageType}
imageType={image?.type}
>
<ImageWrapper
imageSize={imageSize}
imageType={imageType}
imagePosition={
imageUrl !== undefined ? imagePosition : 'top'
}
imagePositionOnMobile={
imageUrl !== undefined ? imagePositionOnMobile : 'top'
}
>
{imageType === 'avatar' && !!avatarUrl ? (
<AvatarContainer
imageSize={imageSize}
imagePosition={imagePosition}
>
<Avatar
imageSrc={avatarUrl}
imageAlt={byline ?? ''}
containerPalette={containerPalette}
format={format}
/>
</AvatarContainer>
) : (
<img src={imageUrl} alt="" role="presentation" />
)}
</ImageWrapper>
{image && (
<ImageWrapper
imageSize={imageSize}
imageType={image.type}
imagePosition={imagePosition}
imagePositionOnMobile={imagePositionOnMobile}
>
{image.type === 'avatar' ? (
<AvatarContainer
imageSize={imageSize}
imagePosition={imagePosition}
>
<Avatar
imageSrc={image.src}
imageAlt={byline ?? ''}
containerPalette={containerPalette}
format={format}
/>
</AvatarContainer>
) : (
<img src={image.src} alt="" role="presentation" />
)}
</ImageWrapper>
)}
<ContentWrapper
imageType={imageType}
imageType={image?.type}
imageSize={imageSize}
imagePosition={imagePosition}
>
Expand Down
4 changes: 2 additions & 2 deletions dotcom-rendering/src/web/components/Card/components/CardLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ const decideWidth = (minWidthInPixels?: number) => {
};

const decidePosition = (
imagePosition: ImagePositionType,
imagePositionOnMobile: ImagePositionType,
imagePosition?: ImagePositionType,
imagePositionOnMobile?: ImagePositionType,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand why these two are changing to optional. In the Props types they remain non-optional: https://github.com/guardian/dotcom-rendering/blob/main/dotcom-rendering/src/web/components/Card/components/CardLayout.tsx#L7-L8

And in Card.tsx above I see this:
image

Copy link
Contributor Author

@mxdvl mxdvl Sep 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You are right, I have now fixed this in b27392f

imageType?: CardImageType,
) => {
if (imageType === 'avatar') {
Expand Down