Support 'preload' attribute for Audio Block #7590

danielbachhuber · 2018-06-27T20:11:36Z

Description

Adds UI for managing preload attribute of the Audio Block:

This is for feature parity with the [audio] shortcode:

The default value is 'None'. Selecting 'Auto' or 'Metadata' will add preload="auto" or preload="meta" to the <audio> element, respectively.

Fixes #6581
Previously #7322

How has this been tested?

Add an Audio Block to the editor. View that it has no preload attribute set by default:

Switch back to Visual mode, select "Auto", and then view that preload='auto' is set:

Save the post and verify the <audio> element includes preload="auto":

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 · 2018-06-27T20:14:06Z

core-blocks/audio/edit.js

+								{
+									name: __( 'None' ),
+									key: 'none',
+									attributeValue: undefined,


attributeValue: undefined,

Worth noting I was stuck on this for a little bit. Originally, I tried attributeValue: false but ended up with preload="false" in the saved <audio> attribute.

If null works I might think that is ever-so-slightly more semantically correct? If it worked I think it might be nicer, but yeah this is fine 😄

null didn't work in my original testing.

danielbachhuber · 2018-06-27T20:16:30Z

Flagged Needs Design Feedback to get approval on this UI:

karmatosed

I think we need to think about the actual UI here. I'll add a note to make a mockup here but let's not merge this as currently we don't use this tab/button combination anywhere and ideally we'd not be adding it. I know WP core does but it's not super effective as a pattern.

One thought though before mocking up, why not a drop down? Can you select more than one or just one? Also what do these settings mean to everyone? I ask as exposing them in the UI like this for the block seems like it needs an explanation.

danielbachhuber · 2018-07-04T00:51:39Z

I'll add a note to make a mockup here but let's not merge this as currently we don't use this tab/button combination anywhere and ideally we'd not be adding it.

Ok, I've moved to the 3.3 milestone.

One thought though before mocking up, why not a drop down? Can you select more than one or just one?

Personally, I prefer the current UI because I can see all potential options. You can only select one option. It's similar to the existing Text Settings UI:

I ask as exposing them in the UI like this for the block seems like it needs an explanation.

It's for feature-parity with the Classic Editor.

karmatosed · 2018-07-05T10:46:30Z

Let's leave it in with tab button UI but also let's add a description as it's kind of not as intuitive what this means compared to text settings.

karmatosed · 2018-07-05T11:01:29Z

Thinking about this a bit more let's combine to one group, no sections and stay with drop down it does make far more sense here.

Also let's have a description if that makes sense or a smaller one can be thought of.

tofumatt

I have a thought about how to label these without needing more description text. I think the issue is we're just being too close to HTML attribute with these labels.

tofumatt · 2018-07-05T11:10:50Z

core-blocks/audio/edit.js

+						<ButtonGroup aria-label={ __( 'Preload' ) }>
+							{ map( [
+								{
+									name: __( 'Auto' ),


I know "auto" is the HTML value for this, but we don't have to follow the HTML attributes to a T.

I think a more instructive/helpful label here would be "Content" (as that's what you're instructing the browser to preload) or, if you want more text: "Preload Content". If you really want the HTML value in brackets you could do "Content (Auto)" but I think "Content would suffice and then nix the need for a description beneath.

I'm not particularly keen to bikeshed the labels at this point.

If you'd like to take the changes through the copy review process, then please do.

I've filed #7726 because it's this way in the classic editor, but I didn't mean to bike shed. I think this existing control text is unclear and this is an easy win.

tofumatt · 2018-07-05T11:11:19Z

core-blocks/audio/edit.js

+									attributeValue: 'auto',
+								},
+								{
+									name: __( 'Metadata' ),


"Metadata only" might be a better label especially if my other suggestion was implemented.

tofumatt · 2018-07-05T11:15:51Z

core-blocks/audio/index.js

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


This will need a deprecated save function, right? (https://github.com/WordPress/gutenberg/blob/master/docs/block-api/deprecated-blocks.md)

Or will this load fine because it's just adding the binary attributes?

The latter, to my knowledge. The markup doesn't change structure; this is only a progressive enhancement.

Oh right, cool. 👍 (Sorry I'm still a bit new to block validation!)

danielbachhuber · 2018-07-05T13:02:18Z

Thinking about this a bit more let's combine to one group, no sections and stay with drop down it does make far more sense here.

@karmatosed My <SelectControl> ends up looking like this:

How do I make it look like yours?

tofumatt · 2018-07-05T13:19:52Z

core-blocks/audio/edit.js

@@ -105,6 +105,7 @@ class AudioEdit extends Component {
 					<SelectControl
 						label={ __( 'Preload' ) }
 						value={ undefined !== preload ? preload : 'none' }
+						// `undefined` is required for the preload attribute to be unset.


Cheers! 👍

karmatosed · 2018-07-05T13:24:15Z

@danielbachhuber no worries, let's see what we can do. How about we start with having it not 'in' the description. You seem to be missing the separator?

karmatosed · 2018-07-05T13:25:06Z

One point to consider would be is this 'advanced' if that's case we could put in with the CSS input class. What do you think?

danielbachhuber · 2018-07-05T13:27:21Z

How about we start with having it not 'in' the description. You seem to be missing the separator?

How do I add a separator?

danielbachhuber · 2018-07-05T13:29:13Z

@karmatosed @tofumatt Could I get clear and specific feedback on how you'd like me to finish this pull request?

tofumatt

Tested locally, including going from a post with existing audio to this one and it all worked for me.

The code makes sense. The removal of PanelBody makes sense to me but it means the "Preload" label is really tight against the Audio block description:

It might make sense to bring it back. Compare it to:

Which I think is nicer. I think that's what @karmatosed is talking about here.

I'd be fine putting "preload" in advanced controls, frankly. Let's just do that–it's not a playback control so restoring the "Playback controls" to how they were before and moving "Preload" under Advanced makes sense to me.

tofumatt · 2018-07-05T14:03:14Z

core-blocks/audio/edit.js

+						label={ __( 'Preload' ) }
+						value={ undefined !== preload ? preload : 'none' }
+						// `undefined` is required for the preload attribute to be unset.
+						onChange={ ( value ) => setAttributes( { preload: 'none' !== value ? value : undefined } ) }


For readability, could you enclose the value here in parens?

This reads, at a glance, like { preload: 'none' } because the constant is first in the conditional check and I think that reduces readability. preload: ( 'none' !== value ? value : undefined ) seems a clear way to indicate this isn't { preload: 'none' }.

tofumatt · 2018-07-05T14:26:23Z

(Sorry, I posted the same screenshot twice if you saw this via email. Updated the review body to show the different ones. I blame Finder for my sort being weird 😉)

danielbachhuber · 2018-07-05T14:46:25Z

t might make sense to bring it back. Compare it to:

Which I think is nicer. I think that's what @karmatosed is talking about here.

@karmatosed Are you 👍 on @tofumatt's proposed implementation?

karmatosed · 2018-07-10T19:56:30Z

All blocks should really have that header block as @tofumatt points out. I also think we can change the order around a bit. What about this?

As to if this is an advanced setting or not.. I am unsure still (happy to be swayed). Let's have what I suggest as a middle ground and iterate from there once the PR is in.

danielbachhuber · 2018-07-11T11:20:53Z

I also think we can change the order around a bit. What about this?

Works for me. The block now looks like this with eed2f1b:

UI is now implemented as requested :-)

tofumatt · 2018-07-11T18:14:11Z

I made a little tweak (d1f7062) because I found the constant-first ternary check was tough to follow. I think we should eschew constant-first styles especially as we're inconsistent about them, but that's not for this issue 😆

If you're okay with that, then 🚢, looks great!

danielbachhuber added [Feature] Blocks Overall functionality of blocks Backwards Compatibility Issues or PRs that impact backwards compatability labels Jun 27, 2018

danielbachhuber added this to the 3.2 milestone Jun 27, 2018

danielbachhuber requested a review from a team June 27, 2018 20:11

danielbachhuber added the Needs Design Feedback Needs general design feedback. label Jun 27, 2018

danielbachhuber commented Jun 27, 2018

View reviewed changes

danielbachhuber force-pushed the 6581-preload-audio branch from 127039f to 90fbdb9 Compare July 3, 2018 11:28

karmatosed previously requested changes Jul 3, 2018

View reviewed changes

danielbachhuber modified the milestones: 3.2, 3.3 Jul 4, 2018

karmatosed removed the Needs Design Feedback Needs general design feedback. label Jul 5, 2018

tofumatt reviewed Jul 5, 2018

View reviewed changes

tofumatt mentioned this pull request Jul 5, 2018

Audio "Preload" settings copy in the editor are unclear #7726

Closed

tofumatt requested changes Jul 5, 2018

View reviewed changes

danielbachhuber added the Needs Design Feedback Needs general design feedback. label Jul 5, 2018

danielbachhuber force-pushed the 6581-preload-audio branch from 33d580d to 41579c7 Compare July 8, 2018 23:57

danielbachhuber added 3 commits July 11, 2018 04:15

Support 'preload' attribute for Audio Block

8e69c8e

Avoid ButtonGroup for preload; collapse controls into one section

bd30049

Comment on the special casing for undefined

3e1cd9d

danielbachhuber force-pushed the 6581-preload-audio branch from 41579c7 to 3e1cd9d Compare July 11, 2018 11:15

Re-arrangle Audio Block controls again

eed2f1b

danielbachhuber requested a review from a team July 11, 2018 11:21

danielbachhuber removed the Needs Design Feedback Needs general design feedback. label Jul 11, 2018

chore: Tweak ternary to add clarity

d1f7062

tofumatt approved these changes Jul 11, 2018

View reviewed changes

danielbachhuber merged commit 4f5af69 into master Jul 11, 2018

danielbachhuber deleted the 6581-preload-audio branch July 11, 2018 19:12

Soean mentioned this pull request Jul 12, 2018

Support 'preload' attribute for Video Block #7929

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support 'preload' attribute for Audio Block #7590

Support 'preload' attribute for Audio Block #7590

danielbachhuber commented Jun 27, 2018

danielbachhuber Jun 27, 2018

tofumatt Jul 5, 2018

danielbachhuber Jul 5, 2018

danielbachhuber commented Jun 27, 2018

karmatosed left a comment •

edited

Loading

danielbachhuber commented Jul 4, 2018

karmatosed commented Jul 5, 2018

karmatosed commented Jul 5, 2018

tofumatt left a comment

tofumatt Jul 5, 2018

danielbachhuber Jul 5, 2018

tofumatt Jul 5, 2018

tofumatt Jul 5, 2018

tofumatt Jul 5, 2018 •

edited

Loading

danielbachhuber Jul 5, 2018

tofumatt Jul 5, 2018

danielbachhuber commented Jul 5, 2018

tofumatt Jul 5, 2018

karmatosed commented Jul 5, 2018

karmatosed commented Jul 5, 2018

danielbachhuber commented Jul 5, 2018

danielbachhuber commented Jul 5, 2018

tofumatt left a comment •

edited

Loading

tofumatt Jul 5, 2018

tofumatt commented Jul 5, 2018

danielbachhuber commented Jul 5, 2018

karmatosed commented Jul 10, 2018 •

edited

Loading

danielbachhuber commented Jul 11, 2018

tofumatt commented Jul 11, 2018

Support 'preload' attribute for Audio Block #7590

Support 'preload' attribute for Audio Block #7590

Conversation

danielbachhuber commented Jun 27, 2018

Description

How has this been tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbachhuber commented Jun 27, 2018

karmatosed left a comment • edited Loading

Choose a reason for hiding this comment

danielbachhuber commented Jul 4, 2018

karmatosed commented Jul 5, 2018

karmatosed commented Jul 5, 2018

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbachhuber commented Jul 5, 2018

Choose a reason for hiding this comment

karmatosed commented Jul 5, 2018

karmatosed commented Jul 5, 2018

danielbachhuber commented Jul 5, 2018

danielbachhuber commented Jul 5, 2018

tofumatt left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt commented Jul 5, 2018

danielbachhuber commented Jul 5, 2018

karmatosed commented Jul 10, 2018 • edited Loading

danielbachhuber commented Jul 11, 2018

tofumatt commented Jul 11, 2018

karmatosed left a comment •

edited

Loading

tofumatt Jul 5, 2018 •

edited

Loading

tofumatt left a comment •

edited

Loading

karmatosed commented Jul 10, 2018 •

edited

Loading