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

Force video player in flash mode. #3195

Merged
merged 1 commit into from
Apr 4, 2014
Merged

Force video player in flash mode. #3195

merged 1 commit into from
Apr 4, 2014

Conversation

polesye
Copy link
Contributor

@polesye polesye commented Apr 3, 2014

Ticket: BLD-958
Solution: add cookie edX_video_player_mode with flash value to force video player in flash mode.

@auraz , @valera-rozuvan please review.

* it returns `false`
*/
function isHtml5Mode() {
return this.getPlayerMode() === 'html5';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not isMode(mode_name)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By using current helpers you shouldn't remember possible values for the player mode.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

@auraz
Copy link
Contributor

auraz commented Apr 3, 2014

Am I right, that there is no way to use this code to force flash mode in manual tests?

@polesye
Copy link
Contributor Author

polesye commented Apr 3, 2014

Just set the cookie.

@auraz
Copy link
Contributor

auraz commented Apr 3, 2014

I will try then)

@auraz
Copy link
Contributor

auraz commented Apr 3, 2014

Works fine manually.
Good job! 👍

@polesye
Copy link
Contributor Author

polesye commented Apr 3, 2014

@valera-rozuvan please review.

@@ -234,6 +234,7 @@ function (Sjson, AsyncProcess) {
};
}

this.el.removeClass('is-captions-rendered');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@polesye Please write a comment what is being hidden here, and why. Is it the entire transcripts panel?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing is being hidden. Just a class that provides us some information about video player.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@polesye I didn't know this. When I look at the code in two month, I will now know why we are doing this. Please write a comment what information is provided and where it is used. For example do we use this in tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the name speaks for itself. Yes, it is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@polesye Do you mind if I add a comment for this line of code, and commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you

@valera-rozuvan
Copy link
Contributor

@polesye I have tried running this branch locally in Chrome, and I get the following JavaScript error:

Uncaught TypeError: Cannot call method 'removeClass' of undefined

@polesye
Copy link
Contributor Author

polesye commented Apr 4, 2014

I think it's because latest rebase with conflicts. I'll fix it.

@polesye
Copy link
Contributor Author

polesye commented Apr 4, 2014

Try it now.

@valera-rozuvan
Copy link
Contributor

@polesye The error

Uncaught TypeError: Cannot call method 'removeClass' of undefined

is not present any longer. Cool.

@valera-rozuvan
Copy link
Contributor

@polesye I have added a comment. Please see 266ce6ebefb655a7f8c7afff6dbb0cb8d0bdf844 commit.

@valera-rozuvan
Copy link
Contributor

@polesye I have checked locally. Video player works as expected in Firefox (Flash, and HTML5 modes), and in Chrome. Code looks good. 👍 Good to merge!

@valera-rozuvan
Copy link
Contributor

@polesye Removed my commit 266ce6ebefb655a7f8c7afff6dbb0cb8d0bdf844 from this PR. Sorry for the troubles! = )

@valera-rozuvan
Copy link
Contributor

@polesye Please squash commits before merging.

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