-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 'preload' attribute for Audio Block #7590
Conversation
core-blocks/audio/edit.js
Outdated
{ | ||
name: __( 'None' ), | ||
key: 'none', | ||
attributeValue: undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null
didn't work in my original testing.
127039f
to
90fbdb9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Ok, I've moved to the 3.3 milestone.
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:
It's for feature-parity with the Classic Editor. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
core-blocks/audio/edit.js
Outdated
<ButtonGroup aria-label={ __( 'Preload' ) }> | ||
{ map( [ | ||
{ | ||
name: __( 'Auto' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
core-blocks/audio/edit.js
Outdated
attributeValue: 'auto', | ||
}, | ||
{ | ||
name: __( 'Metadata' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Metadata only" might be a better label especially if my other suggestion was implemented.
return ( | ||
<figure> | ||
<audio controls="controls" src={ src } autoPlay={ autoplay } loop={ loop } /> | ||
<audio controls="controls" src={ src } autoPlay={ autoplay } loop={ loop } preload={ preload } /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The latter, to my knowledge. The markup doesn't change structure; this is only a progressive enhancement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, cool. 👍 (Sorry I'm still a bit new to block validation!)
@karmatosed My How do I make it look like yours? |
core-blocks/audio/edit.js
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers! 👍
@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? |
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? |
How do I add a separator? |
@karmatosed @tofumatt Could I get clear and specific feedback on how you'd like me to finish this pull request? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
core-blocks/audio/edit.js
Outdated
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 } ) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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' }
.
(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 😉) |
@karmatosed Are you 👍 on @tofumatt's proposed implementation? |
33d580d
to
41579c7
Compare
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. |
41579c7
to
3e1cd9d
Compare
Works for me. The block now looks like this with eed2f1b: |
UI is now implemented as requested :-)
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! |
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"
orpreload="meta"
to the<audio>
element, respectively.Fixes #6581
Previously #7322
How has this been tested?
preload
attribute set by default:preload='auto'
is set:<audio>
element includespreload="auto"
:Types of changes
Enhancement to existing feature.
Checklist: