refactor(OGCard): adjust layout and migrate to TailwindCSS#13355
refactor(OGCard): adjust layout and migrate to TailwindCSS#13355
Conversation
- Increase image area height from 9rem to 12rem (min-h-48) for better og:image display with standard 1.91:1 aspect ratio - Replace styled-components with TailwindCSS utility classes - Replace antd Typography with MarqueeText and native elements - Use line-clamp-3 for description truncation - Unify Container component for both card and skeleton states Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: icarus <[email protected]>
![cherry-ai-bot[bot]](https://avatars.githubusercontent.com/in/3013849?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the refactor — this is a clean migration that reduces boilerplate and fixes the og:image cropping issue.\n\nI left 2 non-blocking suggestions:\n1) confirm visual tradeoff is acceptable across portrait/square images\n2) consider unifying the skeleton image height with the live card ( / 192px) for easier future adjustments\n\nOverall LGTM.
| <PreviewImageContainer> | ||
| <GeneratedGraph /> | ||
| </PreviewImageContainer> | ||
| <div className="flex min-h-48 items-center justify-center"> |
There was a problem hiding this comment.
Good call on — this better matches the 1.91:1 og:image standard. One thing to double-check: you switched the image to , which avoids cropping but may leave blank space for non-landscape images. The PR description mentions this alternative was rejected earlier; worth confirming the visual tradeoff is acceptable across common link previews (e.g., portrait vs. square images).
There was a problem hiding this comment.
The visual effect of object-contain is acceptable for now — portrait/square images do leave some blank space, but it's a reasonable tradeoff over cropping.
Thanks for the review!
| <StyledHyperLink> | ||
| <div className="flex min-h-0 flex-col overflow-hidden p-2"> | ||
| <div className="mb-2 flex items-center gap-2"> | ||
| {hostname && <Favicon hostname={hostname} alt={link} />} |
There was a problem hiding this comment.
Nice consolidation of for both card and skeleton states. Minor suggestion: consider extracting the skeleton height () to a constant or using the same utility class for consistency, so future adjustments to image area height don't require updating multiple places.
There was a problem hiding this comment.
Unfortunately Ant Design's Skeleton component doesn't play well with TailwindCSS utility classes, so extracting to a shared constant isn't straightforward here. We're planning to replace Ant Design in the near future, so this will be addressed as part of that migration.
Thanks for the suggestion!
2 participants
What this PR does
Before this PR:
9rem(144px), resulting in a 2.67:1 aspect ratio that heavily crops standard og:image (1.91:1)Typographycomponents for title and descriptionAfter this PR:
min-h-48(192px), giving a ~2:1 aspect ratio that better fits standard og:image dimensions (1200×630, 1.91:1)Typography.TitlewithMarqueeTextcomponent for title overflow handlingTypography.Paragraphwith nativedivusingline-clamp-3for description truncationContainercomponent for both card and skeleton statesFixes #13352
Why we need it and why it was done in this way
The following tradeoffs were made:
min-h-48(192px) as the image height to approximate the 1.91:1 standard og:image ratio within thew-96(384px) container widthThe following alternatives were considered:
object-fit: containinstead of adjusting container size — rejected because it would leave blank space around landscape imagesBreaking changes
None
Special notes for your reviewer
h-64(256px) toh-72(288px) to accommodate the taller image areaChecklist
/gh-pr-review,gh pr diff, or GitHub UI) before requesting review from othersRelease note