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

Medium: update embed method to reflect Medium changes #12170

Merged
merged 8 commits into from
May 8, 2019
Merged

Medium: update embed method to reflect Medium changes #12170

merged 8 commits into from
May 8, 2019

Conversation

jeherve
Copy link
Member

@jeherve jeherve commented Apr 26, 2019 •
edited
Loading

Changes proposed in this Pull Request:

I originally only wanted to make a few PHPCS fixes. In the process, I discovered some issues:

  1. I changed the display of the HTML markup added alongside the embed script. Instead of displaying a text in English all the time, let's make that text translatable. Since part of the English string came from the embed type before, I simplified the string.
  2. I found that the way we detected embed type for Medium collections was not correct anymore, so I updated the regex accordingly.
  3. I created some unit tests to cover the different embed formats.
  4. We now display a message to post authors when an embed can't be displayed.
  5. We do not try to embed Medium Collections anymore as those are not supported. Instead we display a link to the collection.

Matching WordPress.com diff: D27479-code

Testing instructions:

  • Try adding the following to a classic block:
    • Profiles: https://medium.com/@jeherve
    • Collections: https://medium.com/s/user-friendly
    • Stories: https://medium.com/@jeherve/this-is-a-story-19f582daaf5b
    • And all the above in shortcode formats, for example:
  • [medium url="https://medium.com/@jeherve/this-is-a-story-19f582daaf5b" width="100%" border="false" collapsed="true"]
  • Everything should work.
  • Note that when trying to embed collections, you won't see an embed, but instead you will see a link to the collection. See why below.

Limitations

In my tests, I can't seem to be able to embed collections, but I am not sure why.
https://api.medium.com/_/embed/s/user-friendly does not seem correct.

Update: I contacted Medium support about this, and they confirmed that the embeds are not supported anymore. The profile and story embeds still work, but they are not maintained. The Collection embeds do not work anymore, and there are no plans to get it fixed. As a result, I updated our shortcode to return a link instead of an embed in those cases.

Proposed changelog entry for your changes:

  • Medium: update embed type detection for Collections.

@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Feature] Shortcodes / Embeds [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Pri] Normal labels Apr 26, 2019
@jeherve jeherve requested a review from a team as a code owner April 26, 2019 12:39
@jeherve jeherve self-assigned this Apr 26, 2019
@jetpackbot
Copy link

jetpackbot commented Apr 26, 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: May 7, 2019.
Scheduled code freeze: April 30, 2019

Generated by 🚫 dangerJS against 4a76587

@jeherve
Medium: fix all phpcs warnings
f329a82 
Also change the display of the HTML markup added alongside the embed script. Instead of displaying a text in English all the time, let's make that text translatable. Since part of the English string came from the embed type before, I simplified the string.
@jeherve
Medium: update list of examples at the top of the file.
aae128c
@jeherve
Display error message when a URL was not given in Medium shortcode
8590db3
@jeherve
Add unit tests matching the different Medium URL formats avaialble
87a380a
@jeherve
Medium Collections: update regex as URL format has changed.
0084d27 
Medium collections now use the following format:
https://medium.com/s/user-friendly

You can find some more examples here:
https://medium.com/collections

We should consequently update how we determine embed type accordingly.
@jeherve jeherve added [Status] In Progress [Status] Blocked / Hold and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Status] In Progress labels Apr 26, 2019
@jeherve
Copy link
Member Author

jeherve commented Apr 26, 2019

In my tests, I can't seem to be able to embed collections, but I am not sure why.
https://api.medium.com/_/embed/s/user-friendly does not seem correct.

I contacted Medium about it directly to try to find out more. Here is an example of the problem:
https://jeherve.keybase.pub/testing-medium-embeds.html

@jeherve
Tests: display link instead of embed for Medium collections
e47e387 
Medium does not support Collection embeds anymore.
@jeherve
Update version numbers
701a9ce
@jeherve
Medium collections: return link instead of trying to embed
4a76587 
Medium does not support embedding Medium Collections anymore
@jeherve jeherve added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Blocked / Hold labels Apr 30, 2019
@jeherve
Copy link
Member Author

jeherve commented Apr 30, 2019

I contacted Medium about it directly to try to find out more.

They confirmed that the embeds are not supported anymore. The profile and story embeds still work, but they are not maintained. The Collection embeds do not work anymore, and there are no plans to get it fixed. As a result, I updated our shortcode to return a link instead of an embed in those cases.

@jeherve jeherve changed the title Medium: update embed type detection for Collections. Medium: update embed method to reflect Medium changes Apr 30, 2019
@jeherve jeherve added this to the 7.4 milestone Apr 30, 2019
kraftbj
kraftbj approved these changes May 8, 2019
View reviewed changes
Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

This is a nice set of finds. Frustrating about the changes on their end, but only so much we can do about that.

Tests nicely.

@kraftbj kraftbj merged commit b7c745a into master May 8, 2019
@matticbot matticbot added Touches WP.com Files [Status] Needs Changelog and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels May 8, 2019
@kraftbj kraftbj deleted the update/medium-phpcs branch May 8, 2019 17:34
@kraftbj
Copy link
Contributor

kraftbj commented May 8, 2019

Even though this started as a PHPCS update, there's enough additional on it that I'm not going to include it as part of the general PHPCS changelog item.

jeherve added a commit that referenced this pull request May 17, 2019
@jeherve
changelog: add #12170
05aa725
jeherve added a commit that referenced this pull request May 23, 2019
@jeherve
changelog: add #12170
0947e72
jeherve added a commit that referenced this pull request May 27, 2019
@kraftbj @jeherve
7.4 Changelog (#12304)
0fb4436 
* Kick off the changelog

* Add 7.3.1

* Update date and post link

* changelog: add #12219

* changelog: add #12170

* changelog: add #12184

* Changelog: add #12268

* Changelog: add #12081

* Changelog: add #12323

* Changelog: add #12204

* Changelog: add #12269

* Changelog: add #12332

* changelog: add #12339

* changelog: add #12209

* Changelog: add #12319

* Changelog: add #12357

* Changelog: add #12124

* Changelog: add #12373

* Changelog: add #12252

* Changelog: add #12383

* Changelog: add #12372

* changelog: add #12337

* Changelog: add #12290

* Changelog: add #12301

* Changelog: add #12061

* Testing list: add instructions for #12061

* Changelog: add #12393

* Update minimum supported version

See #12287

* Changelog: add #12406

* Testing list: add #12406

* Changelog: add #12277

* Changelog: add #12412

* Changelog: add #11318

* Changelog: add #12328

* Changelog: add #12425

* Changelog: add #12380

* Changelog: add #12428

* Changelog: add #12414

* Changelog: add #12395

* Changelog & Testing list: add #12416, #12417, #12418, and #12348

* changelog: add #12379

* Changelog: add #12341

* changelog: add #12444

* Changelog: add #12434

* Changelog: add #12454

* Changelog: add #12460

* Changelog: add #12463

* Changelog: add #12457

* Changelog / testing list: add #10333

* Changelog: add #12467


Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kraftbj kraftbj kraftbj approved these changes

Assignees

@jeherve jeherve

Labels
[Feature] Shortcodes / Embeds [Pri] Normal [Status] Tested on WP.com Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended [Type] Dotcom Merge
Projects
None yet
Milestone
7.4
Development

Successfully merging this pull request may close these issues.
4 participants
@jeherve @jetpackbot @kraftbj @matticbot