Skip to content

Twitter cards: add new jetpack_twitter_image_default filter#8664

Merged
dereksmart merged 1 commit intoAutomattic:masterfrom
snarfed:twitter_card_image_filter
Feb 27, 2018
Merged

Twitter cards: add new jetpack_twitter_image_default filter#8664
dereksmart merged 1 commit intoAutomattic:masterfrom
snarfed:twitter_card_image_filter

Conversation

@snarfed
Copy link
Copy Markdown
Contributor

@snarfed snarfed commented Jan 30, 2018 •
edited by oskosk
Loading

Twitter cards: add new jetpack_twitter_image_default filter to allow themes and plugins to customize twitter:image when no suitable image is found automatically. It matches the existing jetpack_open_graph_image_default filter.

(Thanks for considering this!)

To test, add this to your theme's functions.php:

function my_twitter_image_default ($url) {
	return 'http://asdf.com/89asdf.gif';
}
add_filter( 'jetpack_twitter_cards_image_default', 'my_twitter_image_default' );

Fetch your site's home page, and a post page that wouldn't otherwise have a Twitter image set. Verify that they both now include <meta name="twitter:image" content="http://asdf.com/89asdf.gif" />.

Proposed changelog entry:

Twitter cards: add new jetpack_twitter_image_default filter to allow themes and plugins to customize twitter:image when no suitable image is found automatically.

@snarfed snarfed requested a review from a team as a code owner January 30, 2018 15:43
@snarfed snarfed changed the title Thanks in advance for considering this! Twitter cards: add new jetpack_twitter_image_default filter Jan 30, 2018
@jeherve jeherve added Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Sharing Post sharing, sharing buttons [Status] Needs Review This PR is ready for review. labels Jan 31, 2018
jeherve
jeherve previously requested changes Jan 31, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

This seems like a good idea. I've just added a few comments of things that could be improved in my opinion.

class.jetpack-twitter-cards.php Outdated
Copy link
Copy Markdown
Member

@jeherve jeherve Jan 31, 2018

Choose a reason for hiding this comment

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

We've passed the code freeze period for 5.8.0. Could you update this to 5.9.0?

class.jetpack-twitter-cards.php Outdated
Copy link
Copy Markdown
Member

@jeherve jeherve Jan 31, 2018

Choose a reason for hiding this comment

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

Could you add @module sharedaddy here, so our codex parser can pick the filter up?

class.jetpack-twitter-cards.php Outdated
Copy link
Copy Markdown
Member

@jeherve jeherve Jan 31, 2018

Choose a reason for hiding this comment

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

What do you think about changing the filter name to jetpack_twitter_cards_image_default to match other filters in that file?

@jeherve jeherve added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Jan 31, 2018
@snarfed snarfed force-pushed the twitter_card_image_filter branch from 7b940f5 to 5f2eff3 Compare January 31, 2018 17:54
@snarfed
Copy link
Copy Markdown
Contributor Author

snarfed commented Jan 31, 2018

thanks for the review! all those sound good; all done.

@jeherve jeherve added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Jan 31, 2018
@snarfed
Twitter cards: add new jetpack_twitter_image_default filter
024a7b4 
Matches existing `jetpack_open_graph_image_default` filter.
@snarfed snarfed force-pushed the twitter_card_image_filter branch from 5f2eff3 to 024a7b4 Compare February 6, 2018 22:44
@oskosk oskosk dismissed jeherve’s stale review February 13, 2018 22:49

Requested changes seem to addressed

@oskosk oskosk added this to the 5.9 milestone Feb 21, 2018
@oskosk
Copy link
Copy Markdown
Contributor

oskosk commented Feb 21, 2018

I've just updated the testing snippet in the PR description because the filter name was not the same one introduced by the code. Correct one is jetpack_twitter_cards_image_default

oskosk
oskosk approved these changes Feb 21, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

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

Tested and works great!

@oskosk oskosk added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Feb 21, 2018
@dereksmart dereksmart merged commit 3a89a3b into Automattic:master Feb 27, 2018
oskosk added a commit that referenced this pull request Feb 27, 2018
@oskosk
Add changelog entry for #8664
fb7b50f
@snarfed
Copy link
Copy Markdown
Contributor Author

snarfed commented Feb 27, 2018

woo, thanks all!

oskosk added a commit that referenced this pull request Feb 27, 2018
@oskosk
Changelog 5.9: running changelog (#8819)
1174ef5 
* update changelog.txt

* Update readme.txt with scaffolding for 5.9 changelog and release draft shortlink

* Add changelog entry for #8243

* Add changelog entry for #8296

* Add changelog entry for #8367

* Add changelog entry for #8686

* Add changelog entry for #8707

* Add changelog entry for #8709 and #8714

* Add changelog entry for #8729

* Add changelog entry for #8777

* Add changelog entry for #8780

* Add changelog entry for #8786

* Add changelog entry for #8787

* Add changelog entry for #8801 #8805 #8832 #8865 and #8804

* Add changelog entry for #8817

* Add changelog entry for #8822

* Add changelog entry for #8823

* Add changelog entry for #8829

* Add changelog entry for #8834

* move some items to major enhancements

* Add changelog entry for #8836

* Add changelog entry for #8839

* Add changelog entry for #8861

* Add changelog entry for #8862

* Add changelog entry for #8863

* Add changelog entry for #8866

* Add changelog entry for #8870

* Add changelog entry for #8874

* Add changelog entry for #8875

* Add changelog entry for #8881

* Add changelog entry for #8890

* Add changelog entry for #8911

* Add changelog entry for #8927

* Add changelog entry for #8931

* Add changelog entry for #8933

* Add changelog entry for #8930

* fix wording

* typo

* minor fixes

* replace partner scripts for Jetpack Start in changelog entry

* Update to-test.md

* Update to-test.md

* minor style fixes to to-test.md

* minor style fixes to to-test.md

* minor fixes on to-test.md

* Add changelog entry for #8868

* Add changelog entry for #8844

* Add changelog entry for #8664

* Add changelog entry for #8935

* Add changelog entry for #8425

* Add changelog entry for #8625
@kraftbj kraftbj removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve left review comments

+1 more reviewer

@oskosk oskosk oskosk approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Sharing Post sharing, sharing buttons Touches WP.com Files

Projects

None yet

Milestone

5.9

Development

Successfully merging this pull request may close these issues.

6 participants

@snarfed @oskosk @jeherve @kraftbj @dereksmart @matticbot