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

Merges changes to the GettyImages shortcode from WordPress.com. #8591

Merged
merged 3 commits into from
Jan 30, 2018

Conversation

zinigor
Copy link
Member

@zinigor zinigor commented Jan 22, 2018

This is a merge of changes from WordPress.com related to the new format of markup returned by GettyImages oembed endpoints. D9498-wpcom is the mirror change.

Fixes #8569

Testing instructions:

Make sure Getty Images shortcode works as expected in both ways:

  • You should be able to insert an embed code from Getty Images;
  • You should be able to use a shortcode like this: [getty src="82278805" width="462" height="370"]

Both ways should result in an embedded image.

Proposed changelog entry for your changes:

  • Updated the Getty Images shortcode to match changes made on their end.

@zinigor zinigor added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Shortcodes / Embeds [Type] Dotcom Merge labels Jan 22, 2018
@zinigor zinigor added this to the 5.8 milestone Jan 22, 2018
@zinigor zinigor requested a review from aduth January 22, 2018 21:13
@zinigor zinigor requested a review from a team as a code owner January 22, 2018 21:13
@jeherve jeherve added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Jan 22, 2018
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.

Looks like something should not have been ported over.

/**
* Context: http://wp.me/p5j4vm-1y7
*
* @hide-in-jetpack
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this made it in when it should not have?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you, removed!

This is a merge of changes from WordPress.com related to the new format of markup returned by GettyImages oembed endpoints. D9498-wpcom is the mirror change.
@zinigor zinigor force-pushed the merge/getty-shortcode branch from 5a107ce to 7ee1e3c Compare January 23, 2018 16:35
aduth
aduth previously requested changes Jan 28, 2018
@@ -27,8 +27,8 @@ function jetpack_getty_enable_embeds( $site = 'wpcom' ) {
: 'wordpress.com';

// Support their oEmbed Endpoint
wp_oembed_add_provider( '#https?://www\.gettyimages\.com/detail/.*#i', "https://embed.gettyimages.com/oembed/?caller=$caller", true );
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes the $caller variable unused? Should probably be removed.

@zinigor
Copy link
Member Author

zinigor commented Jan 29, 2018

Thanks for taking a look, Andrew! I have fixed caller handling.

Copy link
Member

@dereksmart dereksmart left a comment

Choose a reason for hiding this comment

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

Tests well!

@dereksmart dereksmart dismissed aduth’s stale review January 30, 2018 14:37

Addressed feedback

@dereksmart dereksmart merged commit 55e3dea into master Jan 30, 2018
@dereksmart dereksmart deleted the merge/getty-shortcode branch January 30, 2018 14:37
@jeherve jeherve added [Status] Has Changelog and removed [Status] Needs Changelog [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Jan 30, 2018
jeherve added a commit that referenced this pull request Jan 30, 2018
zinigor pushed a commit that referenced this pull request Jan 30, 2018
* Changelog 5.8: create base for changelog.

* Update 5.8 release post link

* fix 5.8 release date

* Updates to plugin description

* Changelog: add #8499

* Changelog: add #8506

* Changelog: add #8509

* Changelog: add #8516

* Changelog: add #8517

* Changelog: add #8523

* Changelog: add #8547

* Changelog: add #8496

* Changelog: add #8584

* Changelog: add #8595

* Changelog: add #8445

* Changelog: add #8431

* Changelog: add #8284

* Changelog: add #8270

* Changelog: add #8124

* Changelog: add #8581

* Changelog: add #8463

* Changelog: add #8568 (#8646)

* Updates to testing list and changelog

* Changelog: add #8443

* Changelog: add #8459

* Changelog: add #8469

* Changelog: add #8464

* Changelog: add #8478 and #8479

* Changelog: add #8483

* Changelog: add #8488

* Changelog: add #8513

* Changelog: add #8555

* Changelog: add #8565

* Changelog: add #8601

* Changelog: add #8612

* Changelog: add first pass at Search items.

* Changelog: add more info to help test Search.

* Changelog: add #8144

* Changelog: add #8313

* Changelog: add #8419

* Changelog: add #8465

* Changelog: add #8515

* Changelog: add #8587

* Changelog: add #8591

* Changelog: add #8659

* Changelog: add #8661

* Changelog: add #8671

* Changelog: add 5.7.1 to archived changelog too.

* Reverted changes to readme, removed entry about backups.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Shortcodes / Embeds [Type] Dotcom Merge [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants