Render video & audio #1798
Render video & audio #1798
Conversation
twinlensreflex
added
articles-av-epic
|
(note: don't forget to change your url from Currently none of the videos are loading, with a 1052 content not available on this device error I haven't yet been able to figure out why.
|
src/app/components/Video/index.jsx
Outdated
| <div>statsDestination: {statsDestination}</div> | ||
| <div>uiLocale: {uiLocale}</div> | ||
| </div> | ||
| <GridItemConstrainedLargeNoMargin> |
There was a problem hiding this comment.
arbitrarily chose this grid item but I'm assuming we want some logic around this.
There was a problem hiding this comment.
The grid item should be moved into the container /containers/Video/index.jsx, rather than being in the component.
This is so that others can use the Video component without our specific page layout requirements.
src/app/containers/Video/index.jsx
Outdated
| @@ -51,6 +51,24 @@ const VideoContainer = ({ blocks }) => { | |||
| }, | |||
| ]; | |||
|
|
|||
| // prettier-ignore | |||
There was a problem hiding this comment.
we need to have prettier-ignore here or prettier will add trailing ,s to the settings which aren't valid json and break the mediaplayer when passed in like that
There was a problem hiding this comment.
Would be good to add this as a comment, so the prettier ignore won't be accidentally removed.
src/app/containers/Video/index.jsx
Outdated
| {` | ||
| function initialiseRequires() { | ||
| requiredScripts = { | ||
| "bump-4":"//emp.bbci.co.uk/emp/bump-4/bump-4" |
There was a problem hiding this comment.
I was requiring jquery here too, based on the SMP embedding docs saying you would need it if you don't have Barlesque on the page.
however jquery isn't needed for bump 4
src/app/containers/Video/index.jsx
Outdated
| `} | ||
| </script> | ||
| <script | ||
| onLoad="initialiseRequires()" |
There was a problem hiding this comment.
This onLoad chaining, along with mediaPlayerSetup(); on line 100 is to get around the asynchronicity of script loading in helmet.
React-helmet team have their own solution in the comments of this issue and I started to try to use it but as the example SMP embedding code which I've used here is designed to be run client side I chose the multi-script tag and onLoad chaining approach.
|
This works as is if I use a pid from the SMP cookbook rather than our (Optimo I assume?) assets; are these videos not available on test? |
|
It turns out the way we've configured the player only works for live videos, currently investigating how to make it work for test/local assets |
|
updated this to address issues @jamesbrumpton found:
|
|
Currently, the captions on the video are set, by default, to |
| duration, | ||
| kind, | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
To address the subtitle issue raised by James, it should be sufficient to add these properties to the mediaPlayerSettings object:
ui: {
subtitles: {
defaultOn: true,
},
locale: {
lang: 'en-GB',
},
},
edited: to make defaultOn true.
src/app/containers/Figure/index.jsx
Outdated
| const renderCaption = (block, type) => | ||
| block ? <Caption block={block} type={type} /> : null; | ||
| const renderCaption = block => | ||
| block ? <Caption block={block} type="image" /> : null; |
There was a problem hiding this comment.
What the reason this has been hardcoded to "image"?
The type was previously passed to Caption container, then to chooseOffscreenText which switched between which type of offscreen text to use for each service.
There was a problem hiding this comment.
The switching logic makes sense in Caption container, that isn't what I'm questioning. What I'm pointing out is that Figure container which calls Caption container is now hardcoded to the string "image" meaning that any use of Figure container is now explicitly getting the image offscreen text, rather than the type it gets passed
There was a problem hiding this comment.
@pjlee11 - @oluoluoxenfree has updated it now to move the declaration of type to the Image/Video containers.
src/app/models/propTypes/index.js
Outdated
| subtitles: shape({ | ||
| defaultOn: bool.isRequired, | ||
| }).isRequired, | ||
| local: shape({ |
There was a problem hiding this comment.
| local: shape({ | |
| locale: shape({ |
Resolves #134
Overall change:
Render video in storybook
Code changes:
If you can't see videos locally, please follow these instructions to update your local hostnames https://github.com/bbc/simorgh/blob/latest/README.md#changing-request-location
Then, run
npm run storybookand visit http://localhost.bbc.com:9001/?path=/story/video-container--video-clip-with-guidance-with-caption