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.

Sign up for GitHub

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

Jump to bottom

Twitter cards: add new jetpack_twitter_image_default filter #8664

Merged
merged 1 commit into from
Feb 27, 2018
Merged

Twitter cards: add new jetpack_twitter_image_default filter #8664

merged 1 commit into from
Feb 27, 2018

Conversation

snarfed
Copy link
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 [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Sharing Post sharing, sharing buttons [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 31, 2018
jeherve
jeherve previously requested changes Jan 31, 2018
View reviewed changes
Copy link
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
/**
* Filter the default Twitter card image, used when no image can be found in a post.
*
* @since 5.8
Copy link
Member

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
@@ -36,6 +36,18 @@ static function twitter_cards_tags( $og_tags ) {
}

if ( ! is_singular() || ! empty( $og_tags['twitter:card'] ) ) {
/**
* Filter the default Twitter card image, used when no image can be found in a post.
*
Copy link
Member

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
*
* @param string $str Default image URL.
*/
$image = apply_filters( 'jetpack_twitter_image_default', '' );
Copy link
Member

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 would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. 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
Contributor Author

snarfed commented Jan 31, 2018

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

@jeherve jeherve added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! 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
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
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 To request a review from fellow Jetpack developers. Label will be renamed soon. 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
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

@oskosk oskosk oskosk approved these changes

@jeherve jeherve jeherve left review comments

Assignees
No one assigned
Labels
[Feature] Sharing Post sharing, sharing buttons Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
5.9
Development

Successfully merging this pull request may close these issues.
6 participants
@snarfed @oskosk @jeherve @kraftbj @dereksmart @matticbot