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.

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

🎉 Add support for small filename in gdocs images #3005

Merged
merged 6 commits into from
Dec 15, 2023
Merged

Conversation

ikesau
Copy link
Member

@ikesau ikesau commented Dec 9, 2023

Adds an optional smallFilename property to the {.image} component in archie.

This small image is assumed to be smaller than the regular image, and will show when the Image component's img tag has a width that is less than or equal to the width of the small image.

e.g.

{.image}
filename: large.png
smallFilename: small.png
{}

This will generate two <source> elements for the Image's <picture> to consider. The first one will only be active when the viewport width is less than 768px, the second source will always be active.

When previewing, it only uses the Digital Ocean CDN to load the original versions of the images (i.e. a srcset with only one URL specified.)

When baking, it makes multiple resizes of each image and includes all of those in the srcset.

images.mp4

Testing guidelines:

You can use the following archie to test it:

{.image}
filename: ike-test-life-expectancy-large.png
smallFilename: ike-test-life-expectancy-small.png
{}

{.image}
filename: ike-test-life-expectancy-large.png
{}

It should work as expected in the preview, with DPR 2.0 and DPR 1.0

And then if you publish it and run node itsJustJavascript/baker/buildLocalBake.js --steps gdocPosts gdriveImages and then yarn startLocalBakeServer

it should work with all the resized versions of those two images as expected http://127.0.0.1:3000/your-article-slug-here.html

@ikesau ikesau requested a review from sophiamersmann December 9, 2023 00:14
@ikesau ikesau self-assigned this Dec 9, 2023
@ikesau ikesau marked this pull request as draft December 11, 2023 14:57
@ikesau
Copy link
Member Author

ikesau commented Dec 11, 2023

This requires some additional work to function correctly for higher device pixel ratios 🙂

Done

@ikesau ikesau marked this pull request as ready for review December 11, 2023 18:09
Copy link
Member

@sophiamersmann sophiamersmann left a comment

Choose a reason for hiding this comment

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

Looks good!

When clicking on the small image, the viewer opens the large version. Do you think that's easy to address? Having the viewer show the small version when appropriate would be neat.

site/gdocs/Image.tsx Outdated Show resolved Hide resolved
site/gdocs/Image.tsx Show resolved Hide resolved
if (smallImage) {
const smallSizes = getSizes(smallImage.originalWidth)
props.push({
media: "(max-width: 768px)",
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, this fixes the main concern I had when looking at the code yesterday. To make sure I understand: The media attribute makes sure that the small image will only ever show up on small screen sizes (<= 768px), regardless of its own size? (This is important because I'll probably render mobile Grapher charts at a resolution that is close to the ideal one)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that's correct 🙂

Which is why the mobile export can be 1536px wide for (DPR 2) but it will still only show on mobiles.

@ikesau ikesau force-pushed the image-small-variant branch from 4fb567a to 098068d Compare December 15, 2023 15:45
@ikesau ikesau merged commit b422e80 into master Dec 15, 2023
15 checks passed
@ikesau ikesau deleted the image-small-variant branch December 15, 2023 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants