Audio block: don't render empty audio tag.#18850
Conversation
| 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.
da3ccc4 to
bbb123a
Compare
|
Rebased and updated the deprecated version of the block to specify the |
|
I'd appreciate a second review here from our blocks experts :P. Maybe @jorgefilipecosta |
jorgefilipecosta
left a comment
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.
bbb123a to
620dc46
Compare
|
Rebased, added fixtures, and changed block to not render anything at all when |
620dc46 to
a15ff7f
Compare
|
Tests failed for no reason related to the PR, so rebased again to rerun tests. |
jorgefilipecosta
left a comment
There was a problem hiding this comment.
LGTM 👍 Thank you for your contribution!
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: