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.

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

Audio block: don't render empty audio tag. #18850

Merged
merged 1 commit into from Dec 16, 2019

Conversation

@ZebulanStanphill
Copy link
Contributor

ZebulanStanphill commented Dec 1, 2019

Description

Fixes #13556. Replacement for #13564; kudos to @Soean. I incorporated the feedback by @youknowriad and used the complete save function in deprecated.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:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.
@@ -8,7 +8,9 @@ export default function save( { attributes } ) {

return (
<figure>
<audio controls="controls" src={ src } autoPlay={ autoplay } loop={ loop } preload={ preload } />
{ src && (

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 2, 2019

Contributor

wonder fi we should just return null for the whole block if there's no src 🤷‍♂

This comment has been minimized.

Copy link
@ZebulanStanphill

ZebulanStanphill Dec 2, 2019

Author Contributor

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"?

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Dec 6, 2019

Member

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 ZebulanStanphill force-pushed the update/dont-render-empty-audio branch from da3ccc4 to bbb123a Dec 3, 2019
@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor Author

ZebulanStanphill commented Dec 3, 2019

Rebased and updated the deprecated version of the block to specify the attributes and supports objects.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Dec 3, 2019

I'd appreciate a second review here from our blocks experts :P. Maybe @jorgefilipecosta

Copy link
Member

jorgefilipecosta left a 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 ZebulanStanphill force-pushed the update/dont-render-empty-audio branch from bbb123a to 620dc46 Dec 6, 2019
@ZebulanStanphill ZebulanStanphill requested review from nerrad and ntwb as code owners Dec 6, 2019
@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor Author

ZebulanStanphill commented Dec 6, 2019

Rebased, added fixtures, and changed block to not render anything at all when src is not provided.

@ZebulanStanphill ZebulanStanphill force-pushed the update/dont-render-empty-audio branch from 620dc46 to a15ff7f Dec 7, 2019
@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor Author

ZebulanStanphill commented Dec 7, 2019

Tests failed for no reason related to the PR, so rebased again to rerun tests.

Copy link
Member

jorgefilipecosta left a comment

LGTM 👍 Thank you for your contribution!

@ZebulanStanphill ZebulanStanphill merged commit cca3dde into master Dec 16, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@ZebulanStanphill ZebulanStanphill deleted the update/dont-render-empty-audio branch Dec 16, 2019
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.