Audio block: don't render empty audio tag. #18850
Conversation
ZebulanStanphill requested review from ajitbohra, jorgefilipecosta, Soean and talldan as code owners
| @@ -8,7 +8,9 @@ export default function save( { attributes } ) { | |||
| return ( | |||
| <figure> | |||
| <audio controls="controls" src={ src } autoPlay={ autoplay } loop={ loop } preload={ preload } /> | |||
| { src && ( | |||
There was a problem hiding this comment.
wonder fi we should just return null for the whole block if there's no src 🤷♂
There was a problem hiding this comment.
If nothing at all was shown, then the caption (if any) provided by the user would also not be shown. I'm not sure whether or not that makes sense. Perhaps it would make the most sense to show some text saying something like "missing audio file"?
There was a problem hiding this comment.
Hi @ZebulanStanphill,
The user cannot provide a caption if an audio file is not selected so I guess we could follow @youknowriad idea and just return null.
Providing a static text may bring problems e.g: internationalization if the user switches site language the block may become invalid.
ZebulanStanphill force-pushed the update/dont-render-empty-audio branch from da3ccc4 to bbb123a Compare
Rebased and updated the deprecated version of the block to specify the |
youknowriad added the [Feature] Media 
I'd appreciate a second review here from our blocks experts :P. Maybe @jorgefilipecosta |
There was a problem hiding this comment.
Hi @ZebulanStanphill,
Thank you for submitting this fix, the PR is looking great 👍
I think we should add a fixture to test the deprecated block similar to this https://github.com/WordPress/gutenberg//blob/31e21443b63d23d585bdd1e5a827963bdb94b71f/packages/e2e-tests/fixtures/blocks/core__pullquote__deprecated-1.html#L1.
The fixtures can be created by creating a file with the deprecated block markup similar to the file above and then executing GENERATE_MISSING_FIXTURES=y npm run test-unit test/integration/full-content/full-content.test.js. Let me know if I can help you with this process.
ZebulanStanphill force-pushed the update/dont-render-empty-audio branch from bbb123a to 620dc46 Compare
Rebased, added fixtures, and changed block to not render anything at all when |
ZebulanStanphill force-pushed the update/dont-render-empty-audio branch from 620dc46 to a15ff7f Compare
Tests failed for no reason related to the PR, so rebased again to rerun tests. |
Description
Fixes #13556. Replacement for #13564; kudos to @Soean. I incorporated the feedback by @youknowriad and used the complete
savefunction indeprecated.js.How has this been tested?
I created an audio block without setting a source on
master. I then switched to this branch and verified that the markup had been updated.Checklist: