-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Update & Expand Meta Tags #49872
base: master
Are you sure you want to change the base?
Update & Expand Meta Tags #49872
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
dc9182d
to
2a53ae7
Compare
Applied requested changes. |
I've also added some additional changes to the opengraph meta tags. I will update the title/description in 15-20 with the additional changes. Edit: updated |
other than that, the other failing checks seem to be either flaky tests, broken tests, or checks which won't run on forks. |
Util::addHeader('meta', ['property' => 'twitter:title', 'content' => $title]); | ||
Util::addHeader('meta', ['property' => 'twitter:description', 'content' => $description]); | ||
Util::addHeader('meta', ['property' => 'twitter:card', 'content' => $hasImagePreview ? 'summary_large_image' : 'summary']); | ||
Util::addHeader('meta', ['property' => 'twitter:image', 'content' => $ogPreview]); |
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.
Why are we adding twitter specific tags? Can’t it read the standard tags?
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.
Why are we adding twitter specific tags? Can’t it read the standard tags?
It can, however imo it doesn't look as nice. yes, technically only the twitter:card
tag is needed as it will fall back to the opengraph tags, however there is no harm in including them. (& there may be some services which only support the twitter tags, or if one of the twitter tags is present, it only uses twitter tags. unsure, as I have not tested this)
Adds the following twitter meta tags - `twitter:card`: `summary_large_image` if the shared file is an image & it has a preview, otherwise `summary` - `twitter:title`: same as `og:title` - `twitter:description`: same as `og:description` - `twitter:image`: same as `og:image` Fixes nextcloud#49871 Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
…pengraph type to website - Adds the following opengraph tags - images: - `og:image:type`: the mimetype of the image file - audio: - `og:audio`: a direct link to the audio file - `og:audio:type`: the mimetype of the audio file - video: - `og:video`: a direct link to the video file - `og:video:type`: the mimetype of the video file - Changes th `og:type` meta tag from `object` (which is not valid) to `website` Signed-off-by: solonovamax <solonovamax@12oclockpoint.com>
Any updates on the review? |
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
Summary
Improves/expands the meta tags for shared files.
og:image:type
: the mimetype of the image file (TODO: should this be added for all shared files? all files will always have anog:image
tag with either the preview or the nc logo)og:audio
: a direct link to the audio fileog:audio:type
: the mimetype of the audio fileog:video
: a direct link to the video fileog:video:type
: the mimetype of the video fileog:type
tag fromobject
(which is not a type that exists, as far as I can tell) towebsite
twitter:title
: same asog:title
twitter:description
: same asog:description
twitter:card
:summary_large_image
if the shared file is an image & it has a preview, otherwisesummary
twitter:image
: same asog:image
Before changes
After changes
Note
I tested this on my personal self-hosted nextcloud, which is running nextcloud
29.0.10
. I did this by just manually editing theapps/files_sharing/lib/DefaultPublicShareTemplateProvider.php
file.Checklist