Skip to content
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

adds various bit of functionality #10

Merged
merged 4 commits into from
Mar 22, 2019
Merged

adds various bit of functionality #10

merged 4 commits into from
Mar 22, 2019

Conversation

moloko
Copy link
Contributor

@moloko moloko commented Feb 16, 2019

plus a bit of a general tidy-up

* proper transcript functionality
* skip to transcript option
* play, pause and ended events (as per adaptlearning/adapt_framework#2344)
* configuration for player controls & fullscreen
* proper use of ComponentModel (as per adaptlearning/adapt_framework#1179)
@moloko
Copy link
Contributor Author

moloko commented Feb 16, 2019

haven't actually tested this yet, just getting the PR setup - will test properly next week

also note there further improvements to come such as switching over to use the new core inview functionality and a11y updates; I've left these for now so that we can do one more Adapt v2 compatible release first

@danielstorey
Copy link
Member

Hey @moloko,

In advance of starting on the Vimeo Component I was chatting with Ollie and we were talking about having a core Transcript View so I wonder if it's worth creating an abstracted view at this point so it can be moved over to core at some point and to avoid duplicating between Media, YouTube and Vimeo.

I'm also keen on abstracting the actual video views from the component view, and also moving these into core/libraries at some point so they can be used by other components/plugins and all have the same API. I.e wouldn't it be nice if we could do

var videoView;
switch(videoType) {
  case 'youtube':
    videoView = new YouTubeView();
    break;
  case 'video':
    videoView = new VimeoView();
    break;
  case 'html5': default:
    videoView = new HTML5VideoView();
    break;
}

videoView.on('play', handlePlay);
videoView.play();

I don't know if you think this is worth doing at this stage though...?

@moloko
Copy link
Contributor Author

moloko commented Feb 28, 2019

@danielstorey these updates are designed to be compatible with FW 2&3 so anything like that wouldn't be applicable here.

I definitely agree it would be worth creating a TranscriptView in core, probably as part of some kind of MediaView that could easily be extended by plugins.

@oliverfoster
Copy link
Member

Just as an aside to the TranscriptView/MediaView implementation. In v6 we'll hopefully have plugin dependencies and the core will be a series of hierarchically stacked/replaceable plugins providing sub-systems for utilisation by other plugins. Any opportunity to create a reliable version of a commonly used behaviour should be taken. At the moment, in lieu of dependent sub-systems, those behaviours should be created in the core adapt_framework - we can then separate them out into separate parts in v6.

@moloko
Copy link
Contributor Author

moloko commented Mar 7, 2019

have dropped the 'skip to transcript' functionality as I realised it really needs the any handlebars helper to work - and that's not available in FW v.2.x

I'll add this back in properly with the next update (which will drop support for FW v2.x)

Ready to review now if anyone wants to?

@moloko
Copy link
Contributor Author

moloko commented Mar 20, 2019

last call for reviews! otherwise I'll merge tomorrow and get started on an FW4 compatible version

@moloko moloko merged commit 1948d96 into master Mar 22, 2019
@moloko moloko deleted the updates branch March 22, 2019 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants