Skip to content
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 Non-OGP template if any of required OG tags are missing from the page #42

Merged

Conversation

Clpsplug
Copy link
Contributor

@Clpsplug Clpsplug commented Jan 28, 2023

Closes #37.

This patch modifies the behavior of create_properties_from_page so that if the page lacks any of the required OGP tags, it will use a Non-OGP template. This patch prevents users from getting a blank link preview when all the previewed page's meta tags are unrelated to OGP.

Disregard the part below, I found a way to test this patch :)
There are currently no tests because I'm struggling to develop a way to test this behavior. Scraping is mocked away in the existing test suits, so adding a test for this patch to any of the existing test suits did not make much sense. get_properties includes fetching the page and extracting the properties, but should we split this method up just so that we can test this patch?

@Clpsplug
Copy link
Contributor Author

Clpsplug commented Feb 1, 2023

Thanks @ysk24ok for #43 hot-fixing the CI! I'll look into this patch as to why the test is crashing now

@Clpsplug
Copy link
Contributor Author

Clpsplug commented Feb 2, 2023

Caught the culprit: an example was missing a required OGP tag og:type. I added it to the test.
That being said, @ysk24ok, I want to discuss whether should og:type be accessible by the template and be cached or not. By that, I mean that in lib/jekyll-linkpreview.rb, I'd do something along the line of:

     class OpenGraphProperties
       @@template_file = 'linkpreview.html'

-      def initialize(title, url, image, description, domain)
+      def initialize(title, type, url, image, description, domain)
         @title = title
+        @type = type
         @url = url
         @image = image
         @description = description

...and include the @type in the hash. I cannot think of any use cases, though, as this value apparently can be anything. So this is just an idea that can be disregarded.

I also found a way to include the OGP/NOGP distinction in the Rake tests, so please take a look.

@ysk24ok
Copy link
Owner

ysk24ok commented Feb 4, 2023

I want to discuss whether should og:type be accessible by the template and be cached or not

Yes I think basically all properties should be accessible from a template and a cache file.
Please add link_type to https://github.com/ysk24ok/jekyll-linkpreview#template-for-pages-where-open-graph-protocol-metadata-exists.
You don't need to add the type variable to the default template.

@Clpsplug
Copy link
Contributor Author

Clpsplug commented Feb 4, 2023

Added as requested! The user should now be able to extract the og:type property via link_type variable.

I also realized that existing users will not get the og:type variable until they bust their caches. The plugin doesn't appear to crash, but a small migration guide may help.

@ysk24ok ysk24ok merged commit f77d459 into ysk24ok:master Feb 4, 2023
@Clpsplug Clpsplug deleted the feature/use_nogp_on_incomplete_ogp_tags branch February 4, 2023 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pages with meta tags, all of whose property attr are unrelated to OG properties ruins the preview
2 participants