Skip to content

Podcast Player: fix Calypso preview compatibility #15191

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
Merged
merged 1 commit into from
Mar 31, 2020
Merged

Podcast Player: fix Calypso preview compatibility #15191

merged 1 commit into from
Mar 31, 2020

Conversation

@marekhrabe
Copy link
Contributor

@marekhrabe marekhrabe commented Mar 30, 2020

Fixes #15047

Changes proposed in this Pull Request:

  • adds a data attribute to the wrapper of the dynamic part of the podcast player so jetpack-iframe-embed.js knows to leave link handling alone (and allows our React component to handle them)

Testing instructions:

  • this is only testable on a sandboxed wpcom simple site
  • sandbox the site and apply this PR together with D41067-code
  • from Calypso, make a new page on your sandboxed site, add the Podcast Player block and click the Preview button
  • as opposed to wp-admin, the preview doesn't open a new tab but rather loads the preview in a modal window
  • inside the window, try to click a podcast episode link — it should start playing instead of opening a new tab

Proposed changelog entry for your changes:

  • none

@marekhrabe marekhrabe requested review from a team and jeherve March 30, 2020 11:40
@marekhrabe marekhrabe self-assigned this Mar 30, 2020
@matticbot
Copy link
Contributor

matticbot commented Mar 30, 2020

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

@jetpackbot
Copy link
Collaborator

jetpackbot commented Mar 30, 2020

Warnings
⚠️

The PR is missing at least one [Status] label. Suggestions: [Status] In Progress, [Status] Needs Review

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against bc27794

@marekhrabe marekhrabe mentioned this pull request Mar 30, 2020
Closed
@marekhrabe marekhrabe added the [Status] Needs Review This PR is ready for review. label Mar 30, 2020
retrofox
retrofox approved these changes Mar 30, 2020
View reviewed changes
Copy link
Contributor

@retrofox retrofox left a comment

Choose a reason for hiding this comment

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

LGTM. Just wondering about the arbitrary name data-jetpack-iframe-ignore. Should we check with folks who have worked on this?

Tested with the patch, before to apply these changes.

image

after applying these changes:

image

@brbrr brbrr added this to the 8.4 milestone Mar 30, 2020
@marekhrabe
Copy link
Contributor Author

marekhrabe commented Mar 30, 2020

Should we check with folks who have worked on this?

That's actually just me since 2017 😅

I agree on questioning the name. I tried to keep it reasonably short but it's not easy to explain it. @jeherve any idea/preference? I don't mind the current name but if anything better comes up, let's use it.

@jeherve jeherve 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 Mar 30, 2020
@jeherve jeherve modified the milestones: 8.4, 8.5 Mar 31, 2020
@matticbot
Copy link
Contributor

matticbot commented Mar 31, 2020

marekhrabe, Your synced wpcom patch D41069-code has been updated.

@marekhrabe marekhrabe merged commit 68581ee into master Mar 31, 2020
@marekhrabe marekhrabe deleted the add/podcast-player-iframe-ignore branch March 31, 2020 10:25
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 31, 2020
@jeherve jeherve modified the milestones: 8.5, 8.4 Mar 31, 2020
@marekhrabe
Copy link
Contributor Author

marekhrabe commented Mar 31, 2020

r205101-wpcom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@retrofox retrofox retrofox approved these changes

@jeherve jeherve jeherve approved these changes

Assignees

@marekhrabe marekhrabe

Labels

[Block] Podcast Player Touches WP.com Files

Projects

None yet

Milestone

8.4

Development

Successfully merging this pull request may close these issues.

Podcast Player: Doesn't work in Preview (Calypso)

7 participants

@marekhrabe @matticbot @jetpackbot @retrofox @jeherve @brbrr