Create content meta tags for Shelf, Book, Chapter, and Page - #2393
Conversation
Update from base/master
|
This PR implements the feature request in #2348 for media embeds that adhere to Open Graph specifications. Most sites that support Open Graph require a title, URL, description, and image. A new method on the Page model has been created to return either the first image in the HTML body of the page or the book cover to which the page belongs. The default for each image relies on the book default image, which returns as base64. I'm not sure how well this will be supported across different sites, so I welcome additional feedback.
|
Abijeet
left a comment
Abijeet
left a comment
There was a problem hiding this comment.
Thank you for your contribution. Please see my comments.
|
|
||
| try { | ||
| $cover = $images->length > 0 ? $images[0]->getAttribute('src') : $this->book->getBookCover(); | ||
| } catch (Exception $err) { |
There was a problem hiding this comment.
We should either add a use \Exception; statement at the top or put \Exception here.
|
|
||
| public function getCoverImage(): string | ||
| { | ||
| $dom = new \DomDocument(); |
There was a problem hiding this comment.
I would put the logic to fetch the first page image in PageContent class, if that returns null, we can return the book cover.
|
I think if we replace HTML with text (as mentioned here: #2393 (comment)) , we can fix the failing tests. |
Updated to use Str::length for entity descriptions. Moved function to get first image in page to PageContent class.
|
Unit tests continue to fail even after changing code to utilize |
For review of meta tag additions as per PR #2393. This commit removes any image guesswork and only uses images that have been set by the author for the specific content. This also adds tests to cover the expected OG tags.
|
Thanks for this @james-geiger. I've now merged and followed this up with 265f5db with some tweaks and testing. I've scaled back the implementation a bit and removed an logic that guesses images, uses default backup options or uses images of related content. I'd prefer to keep this logic minimal and specific to content that has been specifically chose by the user. Would prefer omission of content rather than any potential confusion. Hope that's understandable. Will be part of the next feature release. |
|
@ssddanbrown Absolutely understand the scale back. I think it's worth noting that, because different sites implement this spec differently, many require an image to be "valid" and won't display without an image. This is prevalent on the "social" sites, could be different on others. I mention this not to suggest that a default be enforced, but so that users can be aware. |
3 participants
Draft PR to implement #2348.
To Do: