Skip to content

Audio: Don't render empty audio element#13564

Closed
Soean wants to merge 3 commits intomasterfrom
update/audio-hide-empty-block
Closed

Audio: Don't render empty audio element#13564
Soean wants to merge 3 commits intomasterfrom
update/audio-hide-empty-block

Conversation

@Soean
Copy link
Copy Markdown
Member

@Soean Soean commented Jan 29, 2019

Description

The HTML audio element should just be rendered if there is a source file. We already have the same behavior in the Video block. This could be useful, if you use the Audio Block in a template and don't select a audio file.
For already created Audio blocks with no source file, we need the deprecated function.

Test:

  1. Create a Audio block with no source selected, on the master branch
  2. Apply the PR and there should be no error in the editor after reload
  3. The Audio element should not be rendered on the frontend

Fixes #13556

@Soean Soean added the [Block] Audio Affects the Audio Block label Jan 29, 2019

deprecated: [
{
save: () => ( <figure><audio controls /></figure> ),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This one is weird though :) shouldn't we copy the previous save function as is?

@gziolo gziolo requested review from ajitbohra and gziolo January 30, 2019 08:24
@gziolo gziolo added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label Jan 31, 2019
@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 7, 2019
@codebykat codebykat self-requested a review March 9, 2019 17:07
Copy link
Copy Markdown
Contributor

@codebykat codebykat left a comment

Choose a reason for hiding this comment

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

Hi @Soean! I tested this and verified that it works! ✨

For anyone else looking at this, since it's hooked to post save, not view, the testing steps are a bit off -- you have to save a post with an empty audio block.

I'm curious about the logic of doing it on save rather than on post view -- couldn't we add it to the view in order to fix any pre-existing posts without an audio source?

It also looks like there's one clarifying question from @youknowriad that would need to be addressed before we can move this forward.

@youknowriad youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019
@aduth
Copy link
Copy Markdown
Member

aduth commented Apr 24, 2019

What's the status of this pull request? Are you able to revisit it in mind of the recent review comments?

@aduth aduth added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Apr 24, 2019
@Soean Soean closed this Dec 1, 2019
@youknowriad youknowriad deleted the update/audio-hide-empty-block branch December 2, 2019 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Block] Audio Affects the Audio Block Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Audio Block - Displays on front end with no media linked

6 participants