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

Podcast Player: Fix missing replace button #15508

Merged
merged 2 commits into from
Apr 23, 2020
Merged

Podcast Player: Fix missing replace button #15508

merged 2 commits into from
Apr 23, 2020

Conversation

frontdevde
Copy link
Contributor

Changes proposed in this Pull Request:

As reported in #15459 with Gutenberg 7.9 the Replace button in the podcast player block is not visible anymore. The source of the change could be traced down to this commit.

This PR suggests changing our implementation to be in line with how the same button is handled in MediaReplaceFlow which is used in the image block.

Fixes #15459

Testing instructions:

  • Create a new post and add the Podcast player (beta) block
  • Add a podcast URL
  • See if the replace button is visible
  • Try to replace the podcast URL
Before After
Screenshot 2020-04-21 at 15 58 00 Screenshot 2020-04-21 at 15 58 14

@frontdevde frontdevde self-assigned this Apr 21, 2020
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello frontdevde! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D42206-code before merging this PR. Thank you!
This revision will be updated with each commit to this PR

@jetpackbot
Copy link

jetpackbot commented Apr 21, 2020 •
edited
Loading

Warnings
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

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 cdba2fa

@frontdevde frontdevde requested a review from obenland April 21, 2020 14:04
obenland
obenland previously approved these changes Apr 21, 2020
View reviewed changes
Copy link
Member

@obenland obenland left a comment

Choose a reason for hiding this comment

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

Tested with Gutenberg 7.9.1 and with vanilla Core trunk, both work as expected. Nice!

@obenland obenland added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Team Review labels Apr 21, 2020
@obenland obenland requested a review from a team April 21, 2020 14:38
@obenland obenland mentioned this pull request Apr 21, 2020
Merged
@jeherve jeherve added this to the 8.5 milestone Apr 21, 2020
@jeherve jeherve added [Type] Bug When a feature is broken and / or not performing as intended [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Apr 21, 2020
jeherve
jeherve requested changes Apr 21, 2020
View reviewed changes
Copy link
Member

@jeherve jeherve 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'm afraid we can't use ToolbarGroup just yet. It wasn't part of WordPress 5.3.2, it was only added in 5.4. It consequently causes the block to error out in WP 5.3.2.

@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 Apr 21, 2020
@frontdevde frontdevde 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 Apr 21, 2020
@obenland obenland requested a review from jeherve April 21, 2020 20:37
marekhrabe
marekhrabe approved these changes Apr 22, 2020
View reviewed changes
Copy link
Contributor

@marekhrabe marekhrabe left a comment

Choose a reason for hiding this comment

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

I have tested in WP 5.3.2 with and without the Gutenberg plugin. Works as expected in both cases.

@jeherve jeherve removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Apr 22, 2020
@jeherve jeherve added the [Status] Ready to Merge Go ahead, you can push that green button! label Apr 22, 2020
@frontdevde frontdevde merged commit c9b52ff into master Apr 23, 2020
@frontdevde frontdevde deleted the fix/podcast-player-replace-button branch April 23, 2020 08:09
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Apr 23, 2020
@frontdevde
Copy link
Contributor Author

r206327-wpcom

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

@WunderBart WunderBart WunderBart left review comments

@obenland obenland obenland left review comments

@retrofox retrofox retrofox left review comments

@jeherve jeherve jeherve approved these changes

@marekhrabe marekhrabe marekhrabe approved these changes

Assignees

@frontdevde frontdevde

Labels
[Block] Podcast Player [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
8.5
Development

Successfully merging this pull request may close these issues.

(3P) Podcast Player: "Replace" button is empty
8 participants
@frontdevde @matticbot @jetpackbot @retrofox @marekhrabe @jeherve @obenland @WunderBart