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

Support 'Autoplay' and 'Loop' in Audio Block 'Playback Controls' #7322

Merged
merged 8 commits into from Jun 27, 2018

Conversation

Projects
None yet
5 participants
@danielbachhuber
Member

danielbachhuber commented Jun 15, 2018

Description

Introduces support for 'Autoplay' and 'Loop' to the Audio Block:

image

Notably, autoplay and loop are only applied on the frontend, not within Gutenberg.

See https://wordpress.slack.com/archives/C02QB2JS7/p1529950740000305 for discussion of boolean attribute handling.

See #6581

How has this been tested?

  1. Upload a MP3 file to the Media Library.
  2. Create a new Audio Block from the MP3 file.
  3. Check the "Autoplay" checkbox, flip to HTML mode, and observe the inclusion of the autoplay attribute.
  4. Manually edit <audio> HTML to include a loop attribute.
  5. Flip back to Visual mode and observe the checked state of the Loop checkbox.
  6. Publish the post and view its frontend state to observe the application of the autoplay and loop attributes.

Screenshots

New UI:

image

GIF of the testing instructions:

audiopost

Classic Editor:

image

Types of changes

Enhancement to existing feature.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 15, 2018

The bug I've run into:

blockcrash

Specifically, if I add loop as an attribute to <audio> in HTML mode, I still get the "Modified Externally" error.

I think this might be an issue with https://github.com/aduth/hpq

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 15, 2018

I think this might be an issue with https://github.com/aduth/hpq

Yep aduth/hpq#3

@danielbachhuber danielbachhuber added this to the 3.2 milestone Jun 25, 2018

@danielbachhuber danielbachhuber changed the title from [WIP] Support 'Autoplay' and 'Loop' in Audio Block 'Playback Controls' to Support 'Autoplay' and 'Loop' in Audio Block 'Playback Controls' Jun 25, 2018

@danielbachhuber danielbachhuber requested a review from WordPress/gutenberg-core Jun 25, 2018

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 25, 2018

Thanks to @aduth's help, I sorted out the issue with parsing attributes.

Updated description to include testing instructions. Note: this only includes autoplay and loop, not preload. I'll handle that in a follow-on PR.

Flagging Needs Design Feedback for the inclusion of new checkboxes.

@tofumatt

Approach here is good, but I have some cleanup and DRY requests.

Let me know when it's ready for another review and I'll have a look.

@@ -98,7 +98,11 @@ export function matcherFromSource( sourceConfig ) {
* @return {*} Attribute value.
*/
export function parseWithAttributeSchema( innerHTML, attributeSchema ) {
return hpqParse( innerHTML, matcherFromSource( attributeSchema ) );
const attrValue = hpqParse( innerHTML, matcherFromSource( attributeSchema ) );

This comment has been minimized.

@tofumatt

tofumatt Jun 26, 2018

Member

Can this be attributeValue instead? Abbr r sad 😉

This comment has been minimized.

@danielbachhuber
@@ -98,7 +98,11 @@ export function matcherFromSource( sourceConfig ) {
* @return {*} Attribute value.
*/
export function parseWithAttributeSchema( innerHTML, attributeSchema ) {
return hpqParse( innerHTML, matcherFromSource( attributeSchema ) );
const attrValue = hpqParse( innerHTML, matcherFromSource( attributeSchema ) );
if ( 'attribute' === attributeSchema.source && 'boolean' === attributeSchema.type ) {

This comment has been minimized.

@tofumatt

tofumatt Jun 26, 2018

Member

I know this is the WordPress style but I see both variable first and constant first in Gutenberg. While my preference is variable first, I just wish we were consistent.

That's not specific to this commit though really. I was just working on a file earlier that had the same issue 😆. We just need some better rules is all.

This comment has been minimized.

@tofumatt

tofumatt Jun 26, 2018

Member

Also, I get why this is, but because the function's docstring says we return a value here and this is the only behaviour where we don't rely on hpqParse, can we document what's happening here?

// HTML boolean attributes set to `true` won't have a value set, so return true
// if their value is `''`.

🤷‍♂️

It will just make the next person reading this not have to wonder, for a few seconds as I did, what this does 😄

This comment has been minimized.

@danielbachhuber

danielbachhuber Jun 27, 2018

Member

Added a note in db11031

@@ -105,6 +105,45 @@ describe( 'block parser', () => {
);
expect( value ).toBe( 'chicken' );
} );
it( 'should return the matcher\'s string attribute value', () => {

This comment has been minimized.

@tofumatt

tofumatt Jun 26, 2018

Member

Seeing this has motivated me to work on #7366 after reviewing this 😆

@@ -53,6 +60,14 @@ class AudioEdit extends Component {
this.setState( { editing: false } );
};
const onToggleAutoplay = ( newVal ) => {

This comment has been minimized.

@tofumatt

tofumatt Jun 26, 2018

Member

Can we use newValue here instead of newVal?

This comment has been minimized.

@danielbachhuber
setAttributes( { autoplay: newVal } );
};
const onToggleLoop = ( newVal ) => {

This comment has been minimized.

@tofumatt

tofumatt Jun 26, 2018

Member

Can we use newValue here instead of newVal?

This comment has been minimized.

@danielbachhuber
setAttributes( { autoplay: newVal } );
};
const onToggleLoop = ( newVal ) => {

This comment has been minimized.

@tofumatt

tofumatt Jun 26, 2018

Member

Seems like it'd be nicer to have:

toggleAttribute = ( attribute ) => ( newValue ) => {
  setAttributes( { [ attribute ]: newValue } );
}

and then have onChange={ toggleAttribute( 'loop' ) }

This comment has been minimized.

@tofumatt

tofumatt Jun 26, 2018

Member

And also: why is this function set as a constant inside every render() call? This should be a component method.

This comment has been minimized.

@danielbachhuber

@danielbachhuber danielbachhuber requested a review from tofumatt Jun 27, 2018

@tofumatt

Tested locally and looks good to me! 🚢

<figure className={ className }>
<audio controls="controls" src={ src } />
{ ( ( caption && caption.length ) || !! isSelected ) && (
<RichText
tagName="figcaption"
placeholder={ __( 'Write caption…' ) }
value={ caption }
onChange={ ( value ) => setAttributes( { caption: value } ) }
onChange={ this.toggleAttribute( 'caption' ) }

This comment has been minimized.

@tofumatt

tofumatt Jun 27, 2018

Member

Nice! 👍

This comment has been minimized.

@tofumatt

tofumatt Jun 27, 2018

Member

Oh, wait, it just occurs to me that this isn't a toggle. I hastily read through this part of the diff.

I don't think this should be using the method toggleAttribute because it makes it read like this is a binary setting control (eg a checkbox or toggle) when it's actually setting the value of a text field.

If you want to keep it a single function I'd choose a more representative function name like setAttributeWithComponentValue but that's pretty verbose.

I'd just make a new function that explicitly sets the caption here… I think it's good to move away from the inline function, but I don't think it's good to use this name for this event handler.

@karmatosed karmatosed self-requested a review Jun 27, 2018

@karmatosed

As with gallery and other blocks we use switches over checkboxes, can you change those here too please?

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 27, 2018

Feel free to flag me for another review once these are moved to switches. 😄

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 27, 2018

@tofumatt @karmatosed Incorporate the toggle UI:

image

@danielbachhuber danielbachhuber requested review from karmatosed and tofumatt Jun 27, 2018

@tofumatt

Everything else looks great but I just noticed an issue with RichText using the toggle function. I left a note about it; after that good to go.

<figure className={ className }>
<audio controls="controls" src={ src } />
{ ( ( caption && caption.length ) || !! isSelected ) && (
<RichText
tagName="figcaption"
placeholder={ __( 'Write caption…' ) }
value={ caption }
onChange={ ( value ) => setAttributes( { caption: value } ) }
onChange={ this.toggleAttribute( 'caption' ) }

This comment has been minimized.

@tofumatt

tofumatt Jun 27, 2018

Member

Oh, wait, it just occurs to me that this isn't a toggle. I hastily read through this part of the diff.

I don't think this should be using the method toggleAttribute because it makes it read like this is a binary setting control (eg a checkbox or toggle) when it's actually setting the value of a text field.

If you want to keep it a single function I'd choose a more representative function name like setAttributeWithComponentValue but that's pretty verbose.

I'd just make a new function that explicitly sets the caption here… I think it's good to move away from the inline function, but I don't think it's good to use this name for this event handler.

@danielbachhuber

This comment has been minimized.

Member

danielbachhuber commented Jun 27, 2018

@tofumatt I reverted back to an inline function d679123

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 27, 2018

(While in general I dislike inline functions it's more readable than before, we have plenty so removing them is a better thing to do en-masse, and it was like that when you got here, so approved!)

I've dismissed @karmatosed's review because the change was made.

Changes from checkboxes to toggles was made

@danielbachhuber danielbachhuber merged commit 5b90b35 into master Jun 27, 2018

2 checks passed

codecov/project Absolute coverage decreased by -0.08% but relative coverage increased by +30.76% compared to a5d6a53
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@danielbachhuber danielbachhuber deleted the 6581-permit-autoplay-loop branch Jun 27, 2018

@danielbachhuber danielbachhuber referenced this pull request Jun 27, 2018

Merged

Support 'preload' attribute for Audio Block #7590

3 of 4 tasks complete
@mtias

This comment has been minimized.

Contributor

mtias commented Jul 5, 2018

Thanks for the work here!

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