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

mediaelement component needed for audio and video blocks #5240

Closed
anthonyburchell opened this Issue Feb 25, 2018 · 25 comments

Comments

Projects
None yet
6 participants
@anthonyburchell

anthonyburchell commented Feb 25, 2018

Issue Overview

The current implementations of the video and audio blocks simply render out html. We need a mediaelement component to possibly allow backwards compatibility with the Core mediaelement library and also allow us templates to expand on the functionality of these players. This issue was discovered while attempting to build out the Playlist block ( issue #805 ). In order to render the playlist in the same way core does we need to expose the mediaelement library through the wp global. My current repo for issue #805 contains a somewhat working version of the mediaelement component and global. I am in the process of pulling that out and editing the video/audio blocks to utilize the component. I'll make a PR for the component here and we can start work on bettering the video and audio blocks later as mentioned in #4961

Expected Behavior

Usage of the component is as follows

<MediaElement
id="player1"
mediaType="audio"
preload="auto"
controls
width="640"
height="360"
poster=""
sources={JSON.stringify(mediaItems)}
options={JSON.stringify(options)}
tracks={JSON.stringify(mediaItems)}
/>

We can expand on this later to get more stylized and functional

Possible Solution

Working on getting the repo cleaned up and ready for PR

Screenshots / Video

screen shot 2018-02-22 at 10 20 21 am

Related Issues and/or PRs

#805 #4961

My progress on this issue can be tracked at this repo: https://github.com/anthonyburchell/gutenberg/tree/add/mediaelement-component-5420

Todos

  • Tests
  • Documentation

@anthonyburchell anthonyburchell referenced this issue Feb 25, 2018

Open

video block & video in Gutenberg #4961

0 of 2 tasks complete
@Lewiscowles1986

This comment has been minimized.

Lewiscowles1986 commented Feb 25, 2018

What about other CSS & JS?
Yesterday and the day before that I was working on modifying #4710
There's a packaged plugin, a linked repo, several notes and screenshots

It's not a react component like what I believe this is suggesting, but it might be a faster route to market for now.

@anthonyburchell

This comment has been minimized.

anthonyburchell commented Feb 25, 2018

@Lewiscowles1986 I think that's what this will allow us. I spent the night getting the mediaelement stuff separated from #805 If you look at the way the component renders, you'll see that we can expand on the layouts and how this displays.
https://github.com/anthonyburchell/gutenberg/blob/add/5240-mediaelement-component/components/media-element/index.js

I just got this repo in a state where the audio block is updated and utilizing the component. If you have a chance I could use the testing and input. I'm a bit tired and am learning js/react as I go so there are bound to be plenty of obvious mistakes ;) : https://github.com/anthonyburchell/gutenberg/tree/add/5240-mediaelement-component

My goal of this issue is for us to, at the very least, get the mediaelement component exposed and have the audio (as it works today) utilizing it. From there we could expand to different templates/styles in the mediaelement component. For instance, I am already working on the way that this component will handle playlist rendering. All of the object attributes for album art (images), meta, and other atts are now exposed to the player via the mediaItem object.

@Lewiscowles1986

This comment has been minimized.

Lewiscowles1986 commented Feb 25, 2018

would you by chance be using git pull to integrate changes rather than git fetch and git rebase -i {from}?

It's quite hard to read through the changes without squashing commits at > 2-5 commits.

@Lewiscowles1986

This comment has been minimized.

Lewiscowles1986 commented Feb 25, 2018

https://github.com/anthonyburchell/gutenberg/blob/add/5240-mediaelement-component/package-lock.json for example has some artefacts from merging gone awry but most importantly it means lots of files have been touched that shouldn't be (I do this sometimes too)

@Lewiscowles1986

This comment has been minimized.

Lewiscowles1986 commented Feb 25, 2018

Opening in Atom <ctrl>+<shift>+<f> then two << as the find generally helps me to see what's changed so I can git patch without noise and then reapply from scratch.

@anthonyburchell

This comment has been minimized.

anthonyburchell commented Feb 25, 2018

@Lewiscowles1986 I totally did git pull and messed everything up. Probably going to start from scratch and re apply my changes later today. Thanks for catching that!

@Lewiscowles1986

This comment has been minimized.

Lewiscowles1986 commented Feb 25, 2018

Sorry to hear that bud, happens to everyone. Let me know when it's done and I'll test, attempt to fix tests (I'm 0/1 on jest so far lol)

@anthonyburchell

This comment has been minimized.

anthonyburchell commented Feb 25, 2018

@Lewiscowles1986 I think this fixes it! New branch (ignore the number...I'm dyslexic and totally swapped them): https://github.com/anthonyburchell/gutenberg/tree/add/mediaelement-component-5420

Git is insanely aggravating. Let me know if this works :)

@Lewiscowles1986

This comment has been minimized.

Lewiscowles1986 commented Feb 25, 2018

When I load an existing Gutenberg post

Error

TypeError: Cannot read property 'url' of undefined
    at save (http://localhost/wp-content/plugins/gutenberg-ab/blocks/build/index.js?ver=1519583973:36839:20)
    at getSaveElement (http://localhost/wp-content/plugins/gutenberg-ab/blocks/build/index.js?ver=1519583973:19851:16)
    at BlockListBlock.render (http://localhost/wp-content/plugins/gutenberg-ab/editor/build/index.js?ver=1519583973:24248:82)
    at h (http://localhost/wp-content/plugins/gutenberg-ab/vendor/react-dom.min.3583f8be.js:130:364)
    at beginWork (http://localhost/wp-content/plugins/gutenberg-ab/vendor/react-dom.min.3583f8be.js:134:70)
    at d (http://localhost/wp-content/plugins/gutenberg-ab/vendor/react-dom.min.3583f8be.js:158:393)
    at f (http://localhost/wp-content/plugins/gutenberg-ab/vendor/react-dom.min.3583f8be.js:159:214)
    at g (http://localhost/wp-content/plugins/gutenberg-ab/vendor/react-dom.min.3583f8be.js:159:462)
    at t (http://localhost/wp-content/plugins/gutenberg-ab/vendor/react-dom.min.3583f8be.js:167:3)
    at r (http://localhost/wp-content/plugins/gutenberg-ab/vendor/react-dom.min.3583f8be.js:164:352)

Post content

<!-- wp:paragraph -->
<p>something something </p>
<!-- /wp:paragraph -->

<!-- wp:embed {"url":"https://www.youtube.com/watch?v=Pf9v9BUusHM","type":"video","providerNameSlug":"youtube"} -->
<figure class="wp-block-embed is-type-video is-provider-youtube">
    https://www.youtube.com/watch?v=Pf9v9BUusHM
</figure>
<!-- /wp:embed -->

<!-- wp:shortcode -->
[video mp4="http://localhost/wp-content/uploads/2017/12/BigBuckBunny_320x180.mp4"] https://www.youtube.com/watch?v=Pf9v9BUusHM [audio mp3="http://localhost/wp-content/uploads/2017/12/bensound-buddy.mp3"]
<!-- /wp:shortcode -->

<!-- wp:video -->
<figure class="wp-block-video"><video controls="" src="http://localhost/wp-content/uploads/2017/12/BigBuckBunny_320x180.mp4"></video></figure>
<!-- /wp:video -->

<!-- wp:audio -->
<figure class="wp-block-audio"><audio controls="" src="http://localhost/wp-content/uploads/2017/12/bensound-buddy.mp3"></audio></figure>
<!-- /wp:audio -->

<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->
@Lewiscowles1986

This comment has been minimized.

Lewiscowles1986 commented Feb 25, 2018

It's also not rendering on a blank page or post.

@anthonyburchell

This comment has been minimized.

anthonyburchell commented Feb 26, 2018

Thanks for the details! I'm looking into it. I think it's a matter of the url attribute being saved as src or vice versa. Also might be something I did when manually bringing over my changes when I was wrangling git prior. Will report back soon!

@anthonyburchell

This comment has been minimized.

anthonyburchell commented Mar 8, 2018

My mediaelement-component repo is up to date again @Lewiscowles1986 and the edit/save issues should be solved. I'm hitting one last snag now that I'm very much stuck on. For whatever reason, in the editor, the player renders as expected. But on the front end, it seems that the mediaelement scripts are not getting enqueued. I added mediaelement to the wp-components array in client-assets.php but I have a suspicion this is not being respected on the front end. Any thoughts anyone has would be most helpful. :)

@Lewiscowles1986

This comment has been minimized.

Lewiscowles1986 commented Mar 8, 2018

Hey buddy, it's triggering some ci errors. One from a test, and a heap of code-style.

blocks/test/full-content.js

full post content fixture › core__audio
[1] 
[1]     File 'core__audio.json' does not match expected value:
[1]     
[1]     expect(received).toEqual(expected)
[1]     
[1]     Expected value to equal:
[1]       [{"attributes": {"align": "right", "caption": [], "src": "https://media.simplecast.com/episodes/audio/80564/draft-podcast-51-livePublish2.mp3"}, "innerBlocks": [], "isValid": true, "name": "core/audio", "originalContent": "<figure class=\"wp-block-audio alignright\">
[1]         <audio controls=\"\" src=\"https://media.simplecast.com/episodes/audio/80564/draft-podcast-51-livePublish2.mp3\"></audio>
[1]     </figure>", "uid": "_uid_0"}]
[1]     Received:
[1]       [{"attributes": {"align": "right", "caption": [], "src": "https://media.simplecast.com/episodes/audio/80564/draft-podcast-51-livePublish2.mp3"}, "innerBlocks": [], "isValid": false, "name": "core/audio", "originalContent": "<figure class=\"wp-block-audio alignright\">
[1]         <audio controls=\"\" src=\"https://media.simplecast.com/episodes/audio/80564/draft-podcast-51-livePublish2.mp3\"></audio>
[1]     </figure>", "uid": "_uid_0"}]

blocks/library/audio/index.js

[0]    18:8   error  'RichText' is defined but never used          no-unused-vars
[0]    37:20  error  Missing trailing comma                        comma-dangle
[0]    42:26  error  Missing trailing comma                        comma-dangle
[0]    83:12  error  'align' is assigned a value but never used    no-unused-vars
[0]    83:19  error  'caption' is assigned a value but never used  no-unused-vars
[0]    83:32  error  'album' is assigned a value but never used    no-unused-vars
[0]    83:39  error  'artist' is assigned a value but never used   no-unused-vars
[0]    83:47  error  'image' is assigned a value but never used    no-unused-vars
[0]    83:54  error  'title' is assigned a value but never used    no-unused-vars
[0]    99:2   error  Mixed spaces and tabs                         no-mixed-spaces-and-tabs
[0]   101:25  error  Missing trailing comma                        comma-dangle
[0]   104:6   error  Unexpected console statement                  no-console
[0]   183:14  error  Missing trailing comma                        comma-dangle
[0]   190:11  error  'align' is assigned a value but never used    no-unused-vars
[0]   190:23  error  'album' is assigned a value but never used    no-unused-vars
[0]   190:30  error  'artist' is assigned a value but never used   no-unused-vars
[0]   190:38  error  'id' is assigned a value but never used       no-unused-vars
[0]   190:42  error  'image' is assigned a value but never used    no-unused-vars
[0]   190:49  error  'title' is assigned a value but never used    no-unused-vars
[0]   190:56  error  'caption' is assigned a value but never used  no-unused-vars

components/media-element/index.js

[0]    6:53   error  Block must not be padded by blank lines            padded-blocks
[0]   13:9    error  There must be a space inside this paren            space-in-parens
[0]   13:23   error  'instance' is defined but never used               no-unused-vars
[0]   13:31   error  There must be a space inside this paren            space-in-parens
[0]   17:7    error  There must be a space inside this paren            space-in-parens
[0]   17:8    error  'media' is defined but never used                  no-unused-vars
[0]   17:13   error  There must be a space inside this paren            space-in-parens
[0]   21:11   error  Block must not be padded by blank lines            padded-blocks
[0]   25:4    error  'sources' is assigned a value but never used       no-unused-vars
[0]   25:24   error  There must be a space inside this paren            space-in-parens
[0]   25:38   error  There must be a space inside this paren            space-in-parens
[0]   26:4    error  'tracks' is assigned a value but never used        no-unused-vars
[0]   26:23   error  There must be a space inside this paren            space-in-parens
[0]   26:36   error  There must be a space inside this paren            space-in-parens
[0]   32:17   error  Expected space(s) after '${'                       template-curly-spacing
[0]   32:34   error  There must be a space inside this paren            space-in-parens
[0]   32:35   error  Strings must use singlequote                       quotes
[0]   32:39   error  There must be a space inside this paren            space-in-parens
[0]   32:40   error  Expected space(s) before '}'                       template-curly-spacing
[0]   33:5    error  Expected space(s) after '${'                       template-curly-spacing
[0]   33:22   error  There must be a space inside this paren            space-in-parens
[0]   33:23   error  Strings must use singlequote                       quotes
[0]   33:27   error  There must be a space inside this paren            space-in-parens
[0]   33:28   error  Expected space(s) before '}'                       template-curly-spacing
[0]   35:17   error  Expected space(s) after '${'                       template-curly-spacing
[0]   35:27   error  Expected space(s) before '}'                       template-curly-spacing
[0]   35:37   error  Expected space(s) after '${'                       template-curly-spacing
[0]   35:50   error  Expected space(s) before '}'                       template-curly-spacing
[0]   35:61   error  Expected space(s) after '${'                       template-curly-spacing
[0]   35:75   error  Expected space(s) before '}'                       template-curly-spacing
[0]   35:77   error  Expected space(s) after '${'                       template-curly-spacing
[0]   35:79   error  There must be a space inside this paren            space-in-parens
[0]   35:104  error  Expected space(s) after '${'                       template-curly-spacing
[0]   35:118  error  Expected space(s) before '}'                       template-curly-spacing
[0]   35:125  error  There must be a space inside this paren            space-in-parens
[0]   35:126  error  Expected space(s) before '}'                       template-curly-spacing
[0]   36:6    error  Expected space(s) after '${'                       template-curly-spacing
[0]   36:8    error  There must be a space inside this paren            space-in-parens
[0]   36:42   error  There must be a space inside this paren            space-in-parens
[0]   36:43   error  Expected space(s) before '}'                       template-curly-spacing
[0]   36:44   error  Expected space(s) after '${'                       template-curly-spacing
[0]   36:46   error  There must be a space inside this paren            space-in-parens
[0]   36:74   error  Expected space(s) after '${'                       template-curly-spacing
[0]   36:89   error  Expected space(s) before '}'                       template-curly-spacing
[0]   36:97   error  There must be a space inside this paren            space-in-parens
[0]   36:98   error  Expected space(s) before '}'                       template-curly-spacing
[0]   37:6    error  Expected space(s) after '${'                       template-curly-spacing
[0]   37:17   error  Expected space(s) before '}'                       template-curly-spacing
[0]   39:17   error  Expected space(s) after '${'                       template-curly-spacing
[0]   39:27   error  Expected space(s) before '}'                       template-curly-spacing
[0]   39:37   error  Expected space(s) after '${'                       template-curly-spacing
[0]   39:50   error  Expected space(s) before '}'                       template-curly-spacing
[0]   39:58   error  Expected space(s) after '${'                       template-curly-spacing
[0]   39:69   error  Expected space(s) before '}'                       template-curly-spacing
[0]   40:6    error  Expected space(s) after '${'                       template-curly-spacing
[0]   40:17   error  Expected space(s) before '}'                       template-curly-spacing
[0]   44:10   error  There must be a space inside this paren            space-in-parens
[0]   44:40   error  A space is required after '{'                      react/jsx-curly-spacing
[0]   44:41   error  A space is required after '{'                      object-curly-spacing
[0]   44:59   error  A space is required before '}'                     object-curly-spacing
[0]   44:60   error  A space is required before '}'                     react/jsx-curly-spacing
[0]   44:68   error  There must be a space inside this paren            space-in-parens
[0]   46:2    error  Block must not be padded by blank lines            padded-blocks
[0]   48:22   error  Block must not be padded by blank lines            padded-blocks
[0]   50:9    error  A space is required after '{'                      object-curly-spacing
[0]   50:28   error  A space is required before '}'                     object-curly-spacing
[0]   52:6    error  There must be a space inside this paren            space-in-parens
[0]   52:7    error  Unary operator '!' must be followed by whitespace  space-unary-ops
[0]   52:26   error  There must be a space inside this paren            space-in-parens
[0]   56:6    error  There must be a space inside this paren            space-in-parens
[0]   56:48   error  There must be a space inside this paren            space-in-parens
[0]   57:33   error  There must be a space inside this paren            space-in-parens
[0]   57:48   error  There must be a space inside this paren            space-in-parens
[0]   57:67   error  There must be a space inside this paren            space-in-parens
[0]   59:14   error  There must be a space inside this paren            space-in-parens
[0]   59:36   error  There must be a space inside this paren            space-in-parens
[0]   59:53   error  There must be a space inside this paren            space-in-parens
[0]   59:75   error  There must be a space inside this paren            space-in-parens
[0]   60:12   error  There must be a space inside this paren            space-in-parens
[0]   60:24   error  There must be a space inside this paren            space-in-parens
[0]   60:39   error  There must be a space inside this paren            space-in-parens
[0]   60:51   error  There must be a space inside this paren            space-in-parens
[0]   60:52   error  Missing trailing comma                             comma-dangle
[0]   61:5    error  There must be a space inside this paren            space-in-parens
[0]   62:17   error  There must be a space inside this paren            space-in-parens
[0]   62:18   error  A space is required after '{'                      object-curly-spacing
[0]   62:75   error  A space is required before '}'                     object-curly-spacing
[0]   62:76   error  There must be a space inside this paren            space-in-parens
[0]   65:16   error  There must be a space inside this paren            space-in-parens
[0]   65:17   error  A space is required after '{'                      object-curly-spacing
[0]   65:85   error  A space is required before '}'                     object-curly-spacing
[0]   65:86   error  There must be a space inside this paren            space-in-parens
[0]   69:6    error  There must be a space inside this paren            space-in-parens
[0]   69:24   error  There must be a space inside this paren            space-in-parens
[0]   71:17   error  There must be a space inside this paren            space-in-parens
[0]   71:18   error  A space is required after '{'                      object-curly-spacing
[0]   71:31   error  A space is required before '}'                     object-curly-spacing
[0]   71:32   error  There must be a space inside this paren            space-in-parens

Process

git clone https://github.com/anthonyburchell/gutenberg gutenberg-ab
cd gutenberg-ab
git checkout add/mediaelement-component-5420
composer update
nvm use 8
npm update
npm run ci
composer run lint
git checkout composer.lock package-lock.json
npm run package-plugin

I'm running in-browser tests atm

@anthonyburchell

This comment has been minimized.

anthonyburchell commented Mar 8, 2018

Thanks for that! I need to learn how to run these tests. Oh there it is in "Process" 😅 This is great info! Working on fixing these now.

@Lewiscowles1986

This comment has been minimized.

Lewiscowles1986 commented Mar 8, 2018

It's running without any frontend console or debugging errors, which is aces. There was a problem loading one gutenberg post (which may be a false positive from testing features & block-types).

When adding to another post, video block comes without mediaelement-js, but also without errors. Audio block comes with mediaelement-js, but without effects from a plugin I've authored, that adds mediaelement-js controls for playback speed.

There is a tiny CSS bug in chrome that results in the bottom of the duration displays being "cut-off"

image

@anthonyburchell

This comment has been minimized.

anthonyburchell commented Mar 8, 2018

Awesome news! So yes, I expected video to not be correct for the time being. The video block will probably need to be updated separately. The goal of this issue is to get mediaelement component exposed and have the audio block use it as an example so we can iterate later to style things with features like you have in your plugin. There is conditional logic in the component to accept a video mediaType however the block you have currently is just rendering down to html5 instead of the mediaelement format. Which would look something like this:

<MediaElement
id="player1"
mediaType="video"
preload="auto"
controls
width="640"
height="360"
poster=""
sources={JSON.stringify(mediaItems)}
options={JSON.stringify(options)}
tracks={JSON.stringify(mediaItems)}
/>
@Lewiscowles1986

This comment has been minimized.

Lewiscowles1986 commented Mar 8, 2018

false positive post was failing to load audio block using the following code

<!-- wp:audio /-->

I'm unsure how that got there, but I've checked and Gutenberg does support the audioblock without error

Stock Gutenberg

image

This mediaelement-js edit

image

@karmatosed karmatosed added this to the Merge Proposal milestone Mar 22, 2018

@joemcgill

This comment has been minimized.

Contributor

joemcgill commented Mar 30, 2018

@anthonyburchell – I spent some time looking into the front end issues here and I think some context about how WordPress works currently is going to help you get this sorted.

First of all, WordPress enqueues it's own custom wrapper + styles for MEJS only when necessary. Generally, this happens whenever a shortcode is being rendered that needs MEJS support. See this example from the audio shortcode.

The wp-mediaelement script manages the initialization of MEJS on specific elements—specifically, those that have wp-audio-shortcode or wp-video-shortcode classes, which you can see in this section of the code.

I'm not sure how the audio and video blocks are being registered at the moment, but if we can make use of the render method to enqueue wp-mediaelement scripts and styles, and we add the correct classes in Core, then we should be good to go here.

@Lewiscowles1986

This comment has been minimized.

Lewiscowles1986 commented Mar 30, 2018

@anthonyburchell @joemcgill as much as it may be a large hammer to crack a walnut, I still think #4710 has the simplest way to implement this via the sandbox component. As for front-end rendering, surely it'd be easier to add URL support to the audio and video shortcodes if they don't exist already and work with what is?

@joemcgill

This comment has been minimized.

Contributor

joemcgill commented Apr 2, 2018

@Lewiscowles1986 If I'm understanding correctly, this would have us keeping audio/video embeds stored as shortcodes in Gutenberg, rather than creating first-class Gutenberg blocks. I think that might be an ok short term solution, but seems to me it would be better to implement these as blocks without the need for shortcodes.

@Lewiscowles1986

This comment has been minimized.

Lewiscowles1986 commented Apr 2, 2018

I hear ya @joemcgill. My concern is that this will be yet another divergence that will confuse existing WP users and leave more options, more engineering effort. The only reason I pull most things into shortcodes is that it means

  • No need for Gutenberg to be active for it to render
  • Supports workflow existing users (~30% apparently)
  • Less effort needed for code maintenance or generation (if they exist tiny wrappers only needed)

I do take your point though.

@joemcgill

This comment has been minimized.

Contributor

joemcgill commented Apr 2, 2018

Current shortcodes should continue to work regardless, IMO. But any new audio/video embeds being created from the Gutenberg UI should have a first-class experience.

Perhaps there's a middle ground here where we could make use of the Gutenberg's PHP rendering callback to reuse some of the existing shortcode rendering logic for these blocks so we're not totally forking these elements?

@karmatosed karmatosed modified the milestones: Merge Proposal, Merge Proposal: Media Apr 12, 2018

@mtias

This comment has been minimized.

Contributor

mtias commented May 23, 2018

Perhaps there's a middle ground here where we could make use of the Gutenberg's PHP rendering callback to reuse some of the existing shortcode rendering logic for these blocks so we're not totally forking these elements?

Yes, by registering a callback the original source effectively becomes a shortcode (just more functional out of the box). We just need to see which media attributes should be saved as attributes for better server-side handling (like ID).

@anthonyburchell

This comment has been minimized.

anthonyburchell commented May 30, 2018

@mtias I'm working on this today and have started down the route of using the callback for the playlist block. I think this is the best way to provide a certain level of backwards compatibility. I've gone two routes in my initial attempts to build the playlist block ( #805 ) The first attempt was using pure js but that breaks anything relying on mediaelement, which leads to this issue here being created. I don't think we actually need a mediaelement component for playlists at all, at least for the initial merge of Gutenberg. I think we should use it though for audio and video elements.

@anthonyburchell

This comment has been minimized.

anthonyburchell commented Jun 28, 2018

Going to close this as a mediaelement component is not needed anymore if we have php callbacks available.

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