-
Notifications
You must be signed in to change notification settings - Fork 492
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
use dataset thumbnail if available #5621
use dataset thumbnail if available #5621
Conversation
src/main/webapp/dataset.xhtml
Outdated
@@ -43,7 +43,7 @@ | |||
<meta property="og:title" content="#{DatasetPage.title}" /> | |||
<meta property="og:type" content="article" /> | |||
<meta property="og:url" content="#{DatasetPage.dataverseSiteUrl}/dataset.xhtml?persistentId=#{dataset.globalId}" /> | |||
<meta property="og:image" content="#{DatasetPage.dataverseSiteUrl.concat(resource['images/favicondataverse.png'])}" /> | |||
<meta property="og:image" content="#{DatasetPage.dataset.getDatasetThumbnail() == null ? DatasetPage.dataverseSiteUrl.concat(resource['images/favicondataverse.png']): DatasetPage.dataverseSiteUrl.concat('/api/datasets/:persistentId/thumbnail?persistentId=').concat(DatasetPage.dataset.getGlobalId().asString())}" /> |
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.
@qqmyers -- one question I have about this og:image
file path is if it requires the dataverseSiteUrl
in your installation? In my initial local and AWS tests, it seems that a relative path with just content="#{resource['images/...']}"
to the default image file works with no problem.
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.
@mheppler - I'm not sure how you're testing, but it may be that relative URLs are OK for some uses. I think we went with a full URL based on https://stackoverflow.com/a/9858694 and looking at https://developers.facebook.com/tools/debug/og/object/ which gives an error such as
with a relative URL.
That said, Facebook also complains about the article:author field not being a profile (minimally a URL for the person), and doesn't like images less than 200x200... other things that could/should be fixed but that don't appear to cause trouble in slack for example...
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.
Ah, that might explain the lack of complaints on my end. I haven't checked my local development on the Facebook yet. (That's on today's agenda.) The dataset thumbnail image size was also something I was going to experiment today, so I am glad to hear you've already gone down that route.
Can one of the admins verify this patch? |
FWIW: I just resolve the conflict, but, with the changes to make the supplied images higher resolution, this could/should wait until one can specify a higher resolution dataset thumbnail as in #5679 |
Over at #10220 (comment) mentioned that this PR increases the dataset logo size (that's my understanding anyway), so it may be a solution for this issue: |
IQSS-4894-Open_Graph_metadata
Hi @qqmyers 👋🏼 I was thinking that this being a solution on the JSF it would be better to implement this logic on |
I found a site that can preview the Open Graph images. For example https://orcascan.com/tools/open-graph-validator?url=https%3A%2F%2Fdataverse.harvard.edu%2Fdataset.xhtml%3FpersistentId%3Ddoi%3A10.7910%2FDVN%2FTJCLKP does show the generic Dataverse logo: (Note that it does say "invalid: Your page is missing Open Graph tags, learn more.) |
At least it gives you a nice detail of what is missing 😄 |
I opened #10586 to keep track of this for the SPA |
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.
Looks good 👍🏼. Adding #10586 to keep track of this for the SPA. probably we need to remove this change from the JSF when the endpoint is updated.
New Contributors
Welcome! New contributors should at least glance at CONTRIBUTING.md, especially the section on pull requests where we encourage you to reach out to other developers before you start coding. Also, please note that we measure code coverage and prefer you write unit tests. Pull requests can still be reviewed without tests or completion of the checklist outlined below. Thanks!
Related Issues
Pull Request Checklist