Skip to content

Add fixes for sketch thumbnails and layouts#358

Merged
davepagurek merged 7 commits intomainfrom
fix/sketches
May 21, 2024
Merged

Add fixes for sketch thumbnails and layouts#358
davepagurek merged 7 commits intomainfrom
fix/sketches

Conversation

@davepagurek
Copy link
Copy Markdown
Collaborator

We had a few image issues with sketches:

  • We show some sketches at a larger size, but only have 400x400 thumbnails from OP
  • Some sketches on OP don't have thumbnails at all because the author never saved one

This addresses those issues by using a scheme where we can provide manual screenshots at a higher resolution. When fetching a sketch thumbnail, it will first look in the src/content/sketches/images directory for a file named ${id}.png, and will fall back to the OP thumbnail if it does not find one. I've added high res screenshots for everything that currently shows up on the Community homepage, and all sketches in the curation that currently do not have any screenshot:

image

image

Another tweak I made was on the page for a single sketch. Every sketch was being displayed in a 400x400 iframe, which means most sketches get scrollbars. The OpenProcessing API also lets us see the sketch source code, so I used it to extract the canvas size that the sketch requests, and then use the following logic:

  • If the sketch sizes itself to the window, then I make it fit the site's standard column width and keep a 3:2 aspect ratio always (the same we use for our thumbnails)

    image

  • If the sketch requests a fixed size canvas, then I display the canvas at that exact size (plus 50px vertically for the header bar in the embed), and then use a CSS transform to scale it down to match the column width

    sketch-resize

This also fixes some little things, like how the name of a sketch in the breadcrumbs included the author due to it being on a second line in the title (now I pass in the second line separately so that it can be displayed but not in the breadcrumbs.)

Copy link
Copy Markdown
Collaborator

@Qianqianye Qianqianye left a comment

Choose a reason for hiding this comment

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

Thanks @davepagurek. LGTM! What do you think, @outofambit?

Copy link
Copy Markdown
Collaborator

@outofambit outofambit left a comment

Choose a reason for hiding this comment

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

love parsing to size the iframe appropriately for the sketch. this looks good to me overall. left one comment for how we might be able to simplify the logic some.

it looks like getting the images appropriately sized exposed an issue with the layout logic on the community page, i think that can be handled separately.

one last consideration: i think the images could go in an api/images folder? technically the images are content but it would keep all the open processing sketch logic and content in the same place and would keep the src/content folder only holding actual Astro content collections.

Comment on lines +17 to +21
const width = Math.min(thumbnailDimensions * scale, 1500);
const height = Math.min(
(typeof imgSrc === 'string' ? 1 : (imgSrc as any).height / (imgSrc as any).width) * thumbnailDimensions * scale,
1000,
);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think for these we can pass in thumbnailDimensions when imgSrc is a string and otherwise leave it undefined. Astro should be able to determine the right width and height based on the layout and the local image file. Or perhaps you already tried that and found an issue?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Are you thinking of replacing scale with just a width property that you pass in? Or are you talking more about omitting the manual width and height entirely for non-OP images? (For the latter, it looks like Astro is still requesting the full 1500x1000 image for me, but maybe there's more props I'd need to pass in?)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah right, I think you'd have to do something like

<Image
    src={imgSrc as any}
    alt="Screenshot of sketch"
    widths={[thumbnailDimensions, 800, 1200, 1500]}
    sizes={`(max-width: 767px) ${thumbnailDimensions}, (max-width: 1024px) 800px, (max-width: 1280px) 1200px, 1500px`}
    width={width}
    height={height}
    loading={lazyLoad ? "lazy" : "eager"}
  />

However, this is really just based on page load size, so it doesn't actually match the optimization you're doing based on grid position.

When I tried this out, the layout breaks between 1024px and 1280px, which is between the laptop and desktop breakpoints, so some part of the css must be depending on different intrinsic sizes for the images.

That all being said, perhaps its best to keep what you have instead of going further down this rabbit hole. :)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm happy to have you merge as-is!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Ahhh right, it would be great if it could use the size of the image itself as a breakpoint. Ok sounds good, I'll leave it as is for now!

@davepagurek
Copy link
Copy Markdown
Collaborator Author

Moving to the api folder sounds good to me, just moved the files there.

What was the layout issue on the community page you were mentioning?

@davepagurek davepagurek merged commit 15fe476 into main May 21, 2024
@davepagurek davepagurek deleted the fix/sketches branch May 21, 2024 00:11
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.

3 participants