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

Feature: boxed preview without an image, for non-OG links #23

Merged
merged 17 commits into from
Jun 17, 2020

Conversation

mostafa
Copy link
Contributor

@mostafa mostafa commented Jun 5, 2020

Hi @ysk24ok,

I've added a feature to include a boxed preview of the link, just like in Medium, to your library, and done some cleanups.
Usage:

Issue: And found out that some websites block traffic to metainspector, so it fails to fetch data.

Suggestion: it would be good if we cache the images.

@ysk24ok
Copy link
Owner

ysk24ok commented Jun 14, 2020

Thank you for this PR and sorry for the late reply.
It seems this change breaks tests, could you please let the tests pass?
https://travis-ci.org/github/ysk24ok/jekyll-linkpreview/builds/694973845 (I'm not sure you can see this page.)
You can run tests by bundle exec rake test in your local environment before git push.

@mostafa
Copy link
Contributor Author

mostafa commented Jun 14, 2020

Hey @ysk24ok,
I wrote a test to check if everything works as expected. It works, but I have changed the focus of the test from fetch to get, since fetch now returns a new metainspector object, rather than meta_tags.

@ysk24ok
Copy link
Owner

ysk24ok commented Jun 15, 2020

Thank you for the quick fix, @mostafa .
I tried this feature in a new Jekyll site and confirmed it worked correctly.
But one question, why are there two NonOpenGraphProperties classes?

@mostafa
Copy link
Contributor Author

mostafa commented Jun 15, 2020

You're welcome! 🙂
I have merged things back and forth, and it slipped out of my hand. I'll fix it.

Copy link
Owner

@ysk24ok ysk24ok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thank you for the pull request!

@ysk24ok ysk24ok merged commit 735b39c into ysk24ok:master Jun 17, 2020
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.

None yet

2 participants