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

Instagram: update embed to support new TV URLs #11329

Merged
merged 9 commits into from
Feb 25, 2019
Merged

Instagram: update embed to support new TV URLs #11329

merged 9 commits into from
Feb 25, 2019

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Feb 12, 2019 •
edited
Loading

Changes proposed in this Pull Request:

This updates our custom embed provider to support Instagram TV URLs, just like Core will soon: https://core.trac.wordpress.org/changeset/44486

I also took the opportunity to add some Unit Tests for Instagram, we did not have any before.

Testing instructions:

  • Try adding the following URLs to a brand new post, and see that they all get transformed into embeds on publish:
A classic IG embed

https://www.instagram.com/p/BnMOk_FFsxg/

A TV IG embed

https://www.instagram.com/tv/BkQjCfsBIzi/

A profile alternative picture IG embed

https://www.instagram.com/jeherve/p/BnMO9vRleEx/

A profile alternative video IG embed

https://www.instagram.com/instagram/tv/BkQjCfsBIzi/

An Instagram shortcode

[instagram url="http://instagram.com/p/BnMO9vRleEx/"]

Proposed changelog entry for your changes:

  • Instagram: update embed to support Instagram TV URLs

@jeherve jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Shortcodes / Embeds [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Feb 12, 2019
@jeherve jeherve added this to the 7.1 milestone Feb 12, 2019
@jeherve jeherve self-assigned this Feb 12, 2019
@jeherve jeherve requested a review from a team as a code owner February 12, 2019 18:43
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello jeherve! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D24268-code before merging this PR. Thank you!

@jetpackbot
Copy link

jetpackbot commented Feb 12, 2019 •
edited
Loading

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: February 14, 2019.
Scheduled code freeze: February 7, 2019

Generated by 🚫 dangerJS against 3bef5b3

@matticbot
Copy link
Contributor

jeherve, Your synced wpcom patch D24268-code has been updated.

@jeherve
Instagram: handle images and videos differently in RSS feeds.
ef11ca3 
For images, display the image in the RSS feed with a link to the Instagram page.
For videos, display only a link to the video.
@matticbot
Copy link
Contributor

jeherve, Your synced wpcom patch D24268-code has been updated.

@jeherve
Instagram: remove the 2 potential embed providers instead of one.
9514490 
This way regardless of your version of Core, we'll remove Core's embed before to add our own.
@matticbot
Copy link
Contributor

jeherve, Your synced wpcom patch D24268-code has been updated.

jeherve and others added 4 commits February 13, 2019 18:08
@jeherve
Add missing ? as www is not always there for Instagram URLs.
db19ff0
@jeherve
Make sure we handle URLs both with and without trailing slash.
d2ae66a 
For reference:
- Before change: https://regex101.com/r/A1p1yt/2
- After the change: https://regex101.com/r/A1p1yt/1
@jeherve
Simplify regexes by only capturing the groups we need to use.
0d6a578
@david-binda @jeherve
Fix pre_kses filter replacing iframe and blockquotes for URLs
3bef5b3 
The code for replacing iframe and blockquote for URLs in terms of pre_kses does not really work, as it's
expecting URL in 2nd matching group, which only exists in the first regex. The second one does not even
have 2nd matching group.

In order to make the code work for both regexes, this changeset updates both regexes to contain
instagram.com URL in 1st matching group.

Originally submitted in D24298
@matticbot
Copy link
Contributor

jeherve, Your synced wpcom patch D24268-code has been updated.

brbrr
brbrr approved these changes Feb 25, 2019
View reviewed changes
Copy link
Contributor

@brbrr brbrr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I also took the opportunity to add some Unit Tests for Instagram, we did not have any before.

<3

Works well. Thanks!

@brbrr brbrr added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Feb 25, 2019
@jeherve jeherve merged commit 2171105 into master Feb 25, 2019
@jeherve jeherve deleted the update/instagram-embed branch February 25, 2019 11:55
@ghost ghost removed the [Status] Ready to Merge Go ahead, you can push that green button! label Feb 25, 2019
jeherve added a commit that referenced this pull request Feb 26, 2019
@jeherve
changelog: add #11329
5e5e5f1
@ockham ockham mentioned this pull request Aug 4, 2020
Closed
@MumuDidi MumuDidi mentioned this pull request May 7, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brbrr brbrr brbrr approved these changes

@david-binda david-binda Awaiting requested review from david-binda

Assignees

@jeherve jeherve

Labels
[Feature] Shortcodes / Embeds [Pri] High [Status] Tested on WP.com Touches WP.com Files [Type] Dotcom Merge [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jeherve @matticbot @jetpackbot @brbrr @david-binda