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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3ad7a80
Support 'Autoplay' and 'Loop' in Audio Block 'Playback Controls'
danielbachhuber Jun 15, 2018
f9c8655
Interpret empty string `''` from hpq as `true` for boolean attribute
danielbachhuber Jun 25, 2018
683df3c
Update fixture to accommodate new attributes
danielbachhuber Jun 25, 2018
dbfb283
No abbreviation for variables
danielbachhuber Jun 26, 2018
bec018f
Add a comment clarifying behavior
danielbachhuber Jun 26, 2018
a7d03bc
Refactor to use common callback handler
danielbachhuber Jun 26, 2018
9cec9f3
Use `ToggleControl` over `CheckboxControl` per design
danielbachhuber Jun 27, 2018
d679123
Revert back to inline function for now
danielbachhuber Jun 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion blocks/api/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,14 @@ export function matcherFromSource( sourceConfig ) {
* @return {*} Attribute value.
*/
export function parseWithAttributeSchema( innerHTML, attributeSchema ) {
return hpqParse( innerHTML, matcherFromSource( attributeSchema ) );
const attributeValue = hpqParse( innerHTML, matcherFromSource( attributeSchema ) );
// HTML attributes without a defined value (e.g. <audio loop>) are parsed
// to a value of '' (empty string), so return `true` if we know this should
// be boolean.
if ( 'attribute' === attributeSchema.source && 'boolean' === attributeSchema.type ) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a note in db1103158cba6b4fb6f6f8800a48b78ab9ae968b

return '' === attributeValue;
}
return attributeValue;
}

/**
Expand Down
39 changes: 39 additions & 0 deletions blocks/api/test/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,45 @@ describe( 'block parser', () => {
);
expect( value ).toBe( 'chicken' );
} );

it( 'should return the matcher\'s string attribute value', () => {
Copy link
Member

Choose a reason for hiding this comment

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

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

const value = parseWithAttributeSchema(
'<audio src="#" loop>',
{
type: 'string',
source: 'attribute',
selector: 'audio',
attribute: 'src',
},
);
expect( value ).toBe( '#' );
} );

it( 'should return the matcher\'s true boolean attribute value', () => {
const value = parseWithAttributeSchema(
'<audio src="#" loop>',
{
type: 'boolean',
source: 'attribute',
selector: 'audio',
attribute: 'loop',
},
);
expect( value ).toBe( true );
} );

it( 'should return the matcher\'s false boolean attribute value', () => {
const value = parseWithAttributeSchema(
'<audio src="#" autoplay>',
{
type: 'boolean',
source: 'attribute',
selector: 'audio',
attribute: 'loop',
},
);
expect( value ).toBe( false );
} );
} );

describe( 'getBlockAttribute', () => {
Expand Down
37 changes: 33 additions & 4 deletions core-blocks/audio/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,19 @@
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { IconButton, Toolbar, withNotices } from '@wordpress/components';
import {
IconButton,
PanelBody,
Toolbar,
ToggleControl,
withNotices,
} from '@wordpress/components';
import { Component, Fragment } from '@wordpress/element';
import {
BlockControls,
InspectorControls,
MediaPlaceholder,
RichText,
BlockControls,
} from '@wordpress/editor';

/**
Expand All @@ -23,10 +30,18 @@ class AudioEdit extends Component {
this.state = {
editing: ! this.props.attributes.src,
};

this.toggleAttribute = this.toggleAttribute.bind( this );
}

toggleAttribute( attribute ) {
return ( newValue ) => {
this.props.setAttributes( { [ attribute ]: newValue } );
};
}

render() {
const { caption, src } = this.props.attributes;
const { autoplay, caption, loop, src } = this.props.attributes;
const { setAttributes, isSelected, className, noticeOperations, noticeUI } = this.props;
const { editing } = this.state;
const switchToEditing = () => {
Expand Down Expand Up @@ -86,14 +101,28 @@ class AudioEdit extends Component {
/>
</Toolbar>
</BlockControls>
<InspectorControls>
<PanelBody title={ __( 'Playback Controls' ) }>
<ToggleControl
label={ __( 'Autoplay' ) }
onChange={ this.toggleAttribute( 'autoplay' ) }
checked={ autoplay }
/>
<ToggleControl
label={ __( 'Loop' ) }
onChange={ this.toggleAttribute( 'loop' ) }
checked={ loop }
/>
</PanelBody>
</InspectorControls>
<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' ) }
Copy link
Member

Choose a reason for hiding this comment

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

Nice! 👍

Copy link
Member

Choose a reason for hiding this comment

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

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.

inlineToolbar
/>
) }
Expand Down
16 changes: 14 additions & 2 deletions core-blocks/audio/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,18 @@ export const settings = {
id: {
type: 'number',
},
autoplay: {
type: 'boolean',
source: 'attribute',
selector: 'audio',
attribute: 'autoplay',
},
loop: {
type: 'boolean',
source: 'attribute',
selector: 'audio',
attribute: 'loop',
},
},

supports: {
Expand All @@ -45,10 +57,10 @@ export const settings = {
edit,

save( { attributes } ) {
const { src, caption } = attributes;
const { autoplay, caption, loop, src } = attributes;
return (
<figure>
<audio controls="controls" src={ src } />
<audio controls="controls" src={ src } autoPlay={ autoplay } loop={ loop } />
{ caption && caption.length > 0 && <RichText.Content tagName="figcaption" value={ caption } /> }
</figure>
);
Expand Down
4 changes: 3 additions & 1 deletion core-blocks/test/fixtures/core__audio.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@
"attributes": {
"src": "https://media.simplecast.com/episodes/audio/80564/draft-podcast-51-livePublish2.mp3",
"caption": [],
"align": "right"
"align": "right",
"autoplay": false,
"loop": false
},
"innerBlocks": [],
"originalContent": "<figure class=\"wp-block-audio alignright\">\n <audio controls=\"\" src=\"https://media.simplecast.com/episodes/audio/80564/draft-podcast-51-livePublish2.mp3\"></audio>\n</figure>"
Expand Down