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

Preview picture: use the 1st pic retrieved if no og:image set #3965

Merged
merged 3 commits into from May 26, 2019

Conversation

nicofrand
Copy link
Contributor

@nicofrand nicofrand commented May 13, 2019

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Documentation no
Translation no
CHANGELOG.md no
License MIT

I'd like to have a preview picture for every link I add to my wallabag. Currently only pictures set as og:image are used for the preview picture (unless the link is an image).

This MR parses the content to find the first picture in the content and use it as a preview (unless there already is a preview picture).

I'd like that to be added to the core but maybe you'd prefer if it required an activation from the user (a new boolean)? That's why I did not edit the tests that check there are no preview pictures for some links: I am waiting for your feedback.

@j0k3r j0k3r changed the base branch from master to 2.4 May 20, 2019 11:20
@j0k3r j0k3r added this to the 2.4.0 milestone May 20, 2019
@j0k3r
Copy link
Member

j0k3r commented May 20, 2019

I switched the target branch to be the 2.4 (to avoid to much rebase before releasing it).
You should rebase your branch against the 2.4

About the work done, good job, it's an interesting feature.
I do not think we should put it behind a config.

@nicofrand
Copy link
Contributor Author

Great, I'll try to rebase it this week (There is no "rebase" button in Github?)!

@j0k3r
Copy link
Member

j0k3r commented May 20, 2019

There is no "rebase" button in Github?

There is. But it's only displayed when the branch has conflicts with the target branch. Which isn't the case here.

@nicofrand
Copy link
Contributor Author

Rebased. I'll try to fix the existing tests now.

@nicofrand
Copy link
Contributor Author

Tests fixed :).

@j0k3r
Copy link
Member

j0k3r commented May 21, 2019

You should run php bin/php-cs-fixer fix to fix CS issues

@nicofrand
Copy link
Contributor Author

Sorry, I did not see the lint failing…

Copy link
Member

@Kdecherf Kdecherf left a comment

Choose a reason for hiding this comment

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

LGTM

@Kdecherf Kdecherf merged commit 5c0701b into wallabag:2.4 May 26, 2019
@j0k3r j0k3r mentioned this pull request May 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants