jsr icon indicating copy to clipboard operation
jsr copied to clipboard

feat: Add OGP to package page

Open nakasyou opened this issue 1 year ago • 12 comments

If you merge this PR, you can show package thumbnail in SNS such as X and Discord.

Example thumbnail (@std/path): IMG_2908

nakasyou avatar Apr 20 '24 08:04 nakasyou

I think the image needs a background color; otherwise, it's pretty hard to read in dark mode. image

josh-collinsworth avatar Apr 22 '24 15:04 josh-collinsworth

This is great work overall, and I'm happy to see this. As far as design, however, I want to be sure we're very thorough.

Most importantly, I'd like to see tests across a wide range of packages. What happens when the package namespace and title are wider than the image, for example? Will the version number and "latest" badge wrap to a new line? If they do, will it push other content down? Are there any scenarios where text could overlap?

Same for the description; how does it flow when a package has very long text? Does it still look right when the package doesn't work with as many things, or has a different score or publish date?

Then, aesthetically, there are a few nitpicks I think we could tighten up before this is fully ready (see the below image to help illustrate):

  • The spacing around the edges of the image are inconsistent (left and bottom spacing are the same, top is a little bigger, and right is much bigger)
  • The package name, version, and "latest" badge don't quite line up visually. Ideally, I'd like to see the bottom baseline of the text align with the bottom of the pill shape of the badge.
  • That "latest" badge has a lot more padding on top than on bottom. Ideally, the text should be vertically centered inside it.
  • All the text on the left edge should align (either bring everything to the left to line up with the @, or move the package name to the right to line up with everything else)
  • Can we space the three stat columns (Published/JSR Score/Works with) a little more evenly? Right now there's a lot of unused space between the first and second columns
  • I think all three columns should be left-aligned. Right now, it looks like "Published" and "JSR Score" are center-aligned, and "Works-with" is right-aligned, and I think just making them all left-aligned would be better for consistency.
  • Should the "4 days ago" text be center-aligned with the score and logos, instead of having them all top-aligned? I could go either way on this one.

I don't know exactly how easy or difficult all of this is given that we're just rendering an image on the fly, but I think the more of these we can address, the better the final outcome will be. :)

og-alignment

josh-collinsworth avatar Apr 22 '24 15:04 josh-collinsworth

@josh-collinsworth, thank you for your advice! I worry about OGP design, so your advice is very helpful.

I'll improve it, following your advice.

nakasyou avatar Apr 23 '24 11:04 nakasyou

@josh-collinsworth, I fixed some. Could you tell me what do you think?: IMG_2917

Max name (scope: 20 chars, name: 32 chars) IMG_2918

nakasyou avatar Apr 24 '24 07:04 nakasyou

@nakasyou That's looking much better.

Question: how can I see these generated images on the site? The social share preview seems to still be the default image when I'm attempting to see the preview locally.

josh-collinsworth avatar Apr 26 '24 21:04 josh-collinsworth

@josh-collinsworth

That's looking much better.

Thank you!

Question: how can I see these generated images on the site? The social share preview seems to still be the default image when I'm attempting to see the preview locally.

OGP was in conflict with the default of app.tsx, so I fixed it. Also, OGP URL has to be absolute URL. Should I add code that switching to jsr.test when Development mode?

nakasyou avatar Apr 27 '24 02:04 nakasyou

Also, OGP URL has to be absolute URL. Should I add code that switching to jsr.test when Development mode?

Yes, that'd be great - if you grep for API_ROOT you can see how to wire through an env var that contains the domain being served. To inject the env var in production, you can add "FRONTEND_ROOT" = "https://${var.domain_name}" in frontend_envs in cloud_run_frontend.tf. If you need help with this, let me know and I can do it :)

You can then default the frontend root to http://jsr.test if no env var is passed (like we do for API_ROOT).

lucacasonato avatar Apr 29 '24 10:04 lucacasonato

Hi @lucacasonato, thank you for advice. OGP URL has been changed to the code that uses FRONTEND_ROOT.

To inject the env var in production, you can add "FRONTEND_ROOT" = "https://${var.domain_name}" in frontend_envs in cloud_run_frontend.tf. If you need help with this, let me know and I can do it :)

It's difficult for me to do it. And even if I could do it, I may destroy environment. So could you do it, please?

nakasyou avatar May 01 '24 13:05 nakasyou

Sure!

lucacasonato avatar May 01 '24 13:05 lucacasonato

One more change I'd like to see: it seems how descriptions are truncated a little too aggressively now. Even packages with very short descriptions are getting cut off.

Could we at least allow the description to wrap to two lines or so? Maybe even move the three columns down a little if we need to?

josh-collinsworth avatar May 10 '24 19:05 josh-collinsworth

@josh-collinsworth, sorry for late. Could you tell me what about you think? IMG_2992

nakasyou avatar May 15 '24 11:05 nakasyou

@nakasyou This is looking really good! Thanks for all your work on this! I have two remaining nitpicks, but I think only one is immediately important.

First, and more importantly: can we prevent text from wrapping mid-word in the description? This isn't great for legibility, and it could also cause some unwanted "words" to appear where they shouldn't. (e.g., A package to create a beautiful butt on) 🙈

CleanShot 2024-05-15 at 09 30 01

And second, less importantly: when the package title wraps to a new line, is it possible to keep the version aligned?

CleanShot 2024-05-15 at 09 27 58

I'm much less worried about the second item; I think that can be an edge case we clean up later. But the first is bound to affect quite a few packages, and potentially cause confusion or unwanted appearance.

josh-collinsworth avatar May 15 '24 14:05 josh-collinsworth

Hi @josh-collinsworth, thank you for reviewing.

I resolved first problem, I'm working to resolve the second problem. IMG_3205

nakasyou avatar Jun 08 '24 14:06 nakasyou

@josh-collinsworth, I fixed second problem, that was a bug. IMG_3207

nakasyou avatar Jun 08 '24 14:06 nakasyou

@nakasyou Looks like CI is picking up some formatting issues. deno fmt should fix them.

josh-collinsworth avatar Jun 11 '24 14:06 josh-collinsworth

@josh-collinsworth

Love this and appreciate the work you've put into it. I think this can and should be shipped. 👍

Thank you! I'm glad.

One minor note: it looks like there may be an issue with non-English glyphs. Still, I think we should ship this as-is and file that for future improvements.

I think it is font and line-breaking problem, i see.

deno fmt should fix them.

I run deno fmt. Please re-run workflow for it.

nakasyou avatar Jun 12 '24 05:06 nakasyou

@nakasyou Sorry, looks like there's still one more CI issue: CleanShot 2024-06-12 at 11 24 10@2x

josh-collinsworth avatar Jun 12 '24 16:06 josh-collinsworth

@josh-collinsworth I'm sorry that I missed checking well, I fixed it.

nakasyou avatar Jun 13 '24 05:06 nakasyou