-
Notifications
You must be signed in to change notification settings - Fork 982
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
Removes large pypi image in Slack link unfurling #17282
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I agree these large images are annoying in Slack.
Just have one refactoring suggestion but I think the overall approach is great.
warehouse/packaging/views.py
Outdated
@@ -288,6 +289,7 @@ def release_detail(release, request): | |||
"license": license, | |||
# Additional function to format the attestations | |||
"PEP740AttestationViewer": PEP740AttestationViewer, | |||
"show_share_image": should_show_share_image(request), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it probably makes more sense to pass the user agent here...
"show_share_image": should_show_share_image(request), | |
"user_agent": request.user_agent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - I used the request that's already in the template (lmk if that's not always present). I also added some handling for Request.user_agent is None
in case the user-agent isn't supplied.
webob.Request.user_agent should be a string or None, as per: https://github.com/Pylons/webob/blob/39d5af3c797e7b867f152c2e8c979de42d029403/src/webob/request.py#L1185
Thanks for the review @di! That's made it a bit cleaner.
|
Thinking about this a bit more: I don't think we can actually vary the response here based on the user agent, unfortunately. These pages will almost always be served from our CDN cache, which doesn't currently vary the cache based on the user agent, so we would end up with a mix of pages with/without this image based on who hit the stale cache first and which request made it to the backend. While we could technically vary on the user agent, I don't think we want to do this in practice because it would make our cache size grow to be extremely large. I think we'll need to take a different approach here: either find a way to make the image more reasonable for Slack, or a way to make Slack prefer a different image without having to vary the response, or something else. |
It's not perfect, but I think there are a few other OpenGraph tricks that could be used here:
<meta name="twitter:label1" content="Latest version" />
<meta name="twitter:data1" content="$LATEST_VERSION" />
<meta name="twitter:label2" content="Uploaded on" />
<meta name="twitter:data2" content="$UPLOAD_DATE" /> (I learned about these tags from this post: https://whitep4nth3r.com/blog/level-up-your-link-previews-in-slack/ -- they're technically not OGP tags but appear to be widely supported, including by Slack.) |
One of my pet peeves is that sharing a pypi link in Slack embeds a tall image - this displays at most two pypi links on my 13" screen until the expansion is deleted by the user.
This PR prevents inclusion of the
og:image
meta tag when the user-agent represents the Slack link unfurler. It keeps the existing images for Twitter and other social network embeds.This PR also makes the following changes to the Twitter embed:
It's worth noting that if #2211 is implemented, this should be reverted, ideally with an image height constraint.
Despite it's simplicity, I'm not sure if adding to the project/release view context is the best approach for this. Would you prefer this to be a service?
I'm considering some further expansion to this - adding fields like the github repo link, downloads count and latest release date to the metadata (as seen in this example, using the
twitter:label1
/twitter:data1
meta properties).