Add muted message via direct DOM manipulation #6573
Conversation
|
Plugin builds for 3755725 are ready 🛎️!
|
| * @return {ReactElement} Inspector Controls. | ||
| */ | ||
| export const setUpVideoInspectorControls = ( props ) => { | ||
| const { isSelected, attributes: { autoplay, muted } } = props; | ||
| export const VideoInspectorControls = ( props ) => { |
There was a problem hiding this comment.
This works perfectly but my only concern is how fragile this would be going forward whenever the video block is to be updated in the future. @delawski could you add an E2E test to verify that the message is shown under the muted toggle? That way we can catch whenever something goes awry with the DOM mutation.
There was a problem hiding this comment.
That is a very good idea! I'll take set up an E2E test for that 👍
There was a problem hiding this comment.
An E2E test has been added in 3755725.
|
|
||
| message.current = document.createElement( 'p' ); | ||
| message.current.classList.add( 'amp-video-autoplay-notice' ); | ||
| message.current.textContent = __( 'Autoplay will mute the video player by default in AMP mode.', 'amp' ); |
There was a problem hiding this comment.
| message.current.textContent = __( 'Autoplay will mute the video player by default in AMP mode.', 'amp' ); | |
| message.current.textContent = __( 'Autoplay will cause the video to be muted in many browsers to prevent a poor user experience. It will be muted in AMP for this reason as well.', 'amp' ); |
There was a problem hiding this comment.
Addressed in 9c80afd
|
This appears to be working quite well: muted-notice.mov |
| * we traverse through the DOM tree to finally find out the | ||
| * "Muted" toggle container. | ||
| */ | ||
| const blockInspector = [ ...document.querySelectorAll( '.edit-post-sidebar .block-editor-block-card__description' ) ] |
There was a problem hiding this comment.
QUESTION: why no jQuery?
😉
There was a problem hiding this comment.
Why use jQuery when few vanilla JS do trick? 😉
There was a problem hiding this comment.
It was really hard to resist the temptation 😅
|
I'm merging this one into the previous feature branch so that this can be reviewed as a whole. |
Summary
This is a rather rough alternative to the approach proposed in #6547. Here we're manipulating the DOM imperatively inside the
useLayoutEffecthook in order to inject the message in the correct place.This should be tested quite throughly.
Checklist