Social: Individual scrolling and update social media preview mockups to match current platform#47985
Social: Individual scrolling and update social media preview mockups to match current platform#47985ederrengifo wants to merge 16 commits intotrunkfrom
Conversation
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Super Cache plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
a1435b6 to
ff17582
Compare
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryCoverage changed in 15 files. Only the first 5 are listed here.
2 files are newly checked for coverage.
|
689c41d to
ad0ae4e
Compare
gmjuhasz
left a comment
There was a problem hiding this comment.
Hi! Thanks for the braveness and jumping into this, I went through the code, this needs some refining on the code organization level. On a general note there are some things to be fixed, otherwise I also left specific things below as comments.
- This PR got too big and should be separated into smaller PRs that are more focused on a specific fix/change.
- There are some anti-patterns in the code like
!importantin scss files, which can cause races, we should use stricter selectors or modify those props in the parent if needed. - Every preview component had
max-width: clamp(200px, 100%, Xpx)replaced withwidth: 100%; max-width: Xpx. Theclamp()provided a minimum width of200px, preventing the previews from collapsing too small. The new approach has no minimum width constraint. This could cause layout issues in narrow containers. - There are dead CSS selectors like
.facebook-preview__descriptionstyles remain in style.scss but the element was removed from the component.facebook-preview__inforemain but the element was removed- Portrait/landscape mode classes removed from components but some related CSS may still exist
- There are some hardcoded values without explanation more max-heights and other CSS props, probably we should generalize them as much as possible
projects/plugins/jetpack/_inc/client/traffic/seo.jsxstill passesdescriptionandimageModetoFacebookLinkPreview, but the component no longer uses either prop. The description was displayed to users in the SEO preview — this is a silent feature regression.
projects/js-packages/number-formatters/.duel-cache/dual.708b8660.tsbuildinfo
Outdated
Show resolved
Hide resolved
projects/js-packages/number-formatters/.duel-cache/primary.708b8660.tsbuildinfo
Outdated
Show resolved
Hide resolved
| return ( | ||
| <Fragment key={ entry.id }> | ||
| <Stack direction="column" gap={ labelSpacing }> | ||
| <Stack direction="column" gap={ labelSpacing as number }> |
There was a problem hiding this comment.
This seems like a bandaid that hides a type issue that probably came up? It's in leaderboard-chart package which should be unrelated to the changes
There was a problem hiding this comment.
yeah, this was causing a failure in the checks and Claude suggested to fix it quickly here. Is there any other way to just ignore it?
| __( '1h', 'social-previews' ) | ||
| } | ||
| </span> | ||
| <span>1h</span> |
There was a problem hiding this comment.
This change removes the translation and replaces it with a generic string
| <div className="linkedin-preview__header--profile"> | ||
| <div className="linkedin-preview__header--profile-info"> | ||
| <div className="linkedin-preview__header--profile-name"> | ||
| { name || __( 'Account Name', 'social-previews' ) } |
There was a problem hiding this comment.
The name prop fallback __( 'Account Name', 'social-previews' ) was removed. Now if name is undefined/empty, nothing renders. The • separator and 1st indicator were also removed — are these intentional design changes?
There was a problem hiding this comment.
Restoring both changes, the first one shouldn't have happened, and the second one initially thought as a way to simplify the mockup but in a second thought, would be better if matches better the output.
| <div | ||
| className={ `facebook-preview__description ${ | ||
| compactDescription ? 'is-compact' : '' | ||
| }` } | ||
| > | ||
| { description && facebookDescription( description ) } | ||
| { isArticle && | ||
| ! description && | ||
| // translators: Default description for a Facebook post | ||
| __( 'Visit the post for more.', 'social-previews' ) } | ||
| </div> | ||
| <div className="facebook-preview__info"> | ||
| <FacebookPostIcon name="info" /> | ||
| </div> |
There was a problem hiding this comment.
This removes the full Facebook description block, is that intentional?
There was a problem hiding this comment.
Yes, facebook doesn't seem to show the description anymore as part of the link preview
There was a problem hiding this comment.
This file got messed up quite a bit, I see description was removed here as well, the helper and cardType props also got removed, probably by accident?
There was a problem hiding this comment.
Same as facebook, many link previews are not showing descriptions anymore. However, I understand the regression in terms of SEO so I'm fixing that aspect.
| const img = e.currentTarget; | ||
| const natural = img.naturalWidth / img.naturalHeight; | ||
| // Clamp between 4:5 (portrait) and 1.91:1 (landscape) | ||
| const clamped = Math.max( MIN_RATIO, Math.min( MAX_RATIO, natural ) ); | ||
| setAspectRatio( clamped ); | ||
| }, [] ); |
There was a problem hiding this comment.
The aspect ratio is derived from the image and won't change — this could cause a visual flash/reflow when the image loads and the container resizes from 1:1 to the clamped ratio. I think we should be using CSS aspect-ratio with object-fit instead
There was a problem hiding this comment.
As far as I understand, Instagram doesn't allow that flexibility to accomodate to the image's aspect ratio and forces the images to be shown within a specific one Source. I'm addressing this as a something to avoid the visual flash but still comply with the way the image will be shown based on Instagram rules. Does that make sense? Because the same happen also in other cases where the image has to be shown in specific ratios.
|
Thank you for the review @gmjuhasz ! I will try to fix the issues first and then split it into smaller PRs. Do you suggest a PR for each mockup case? My first thinking was two PRs for the scrolling and the mockups but I also notice the changes in the mockups are not small. |
|
I addressed the feedback and here is a quick summary of the latest updates: Bug fixes:
Code quality:
Instagram aspect ratio:
Tumblr attachment: (Not part of the feedback but found the issue as I was testing)
Twitter card border:
|
Fixes SOCIAL-432 SOCIAL-433
Proposed changes
Scrolling adjusment
Adjustments in preview mockups
Adjusted Facebook mockup
Adjusted Twitter/X mockup
Adjusted LinkedIn
Adjusted Instagram
Adjusted Tumblr
Adjusted Mastodon
Adjusted Bluesky
Adjusted Threads
Other information
None
Related product discussion/links
p1HpG7-xW0
Does this pull request change what data or activity we track or use?
No
Testing instructions
Use the docs for useful testing snippets.
For the scrolling:
For the mockups: