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

Bug/relocate django vars #117

Merged
merged 9 commits into from
Feb 13, 2017
Merged

Bug/relocate django vars #117

merged 9 commits into from
Feb 13, 2017

Conversation

dorosh
Copy link

@dorosh dorosh commented Feb 13, 2017

No description provided.

@dorosh dorosh force-pushed the bug/relocate-django-vars branch 2 times, most recently from c9e3f8b to 864a035 Compare February 13, 2017 14:43
Copy link

@z4y4ts z4y4ts left a comment

Choose a reason for hiding this comment

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

Now much better. Only two minor things to address. and it's good to go.

}; // eslint-disable-line

var transcripts = {};
player_state_obj.transcripts.forEach(function loop_transcript(transcript) {
Copy link

Choose a reason for hiding this comment

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

Use camelCase - loopTranscript

@@ -105,10 +106,24 @@ def metadata_fields(self):
"""
return []

def _extra_context(self, context):
Copy link

Choose a reason for hiding this comment

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

Why new function and new context['player_state_json']?

@@ -5,6 +5,7 @@
Base Video player plugin
"""

import json
Copy link

Choose a reason for hiding this comment

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

Please order section alphabetically.

@dorosh dorosh force-pushed the bug/relocate-django-vars branch from 864a035 to 18f939f Compare February 13, 2017 14:58
-add valid json into player context on backend
-parse json on frontend and use js object as global var
@dorosh dorosh force-pushed the bug/relocate-django-vars branch from 18f939f to 7adf6b5 Compare February 13, 2017 15:00
@OPersian OPersian self-requested a review February 13, 2017 16:04
Copy link

@OPersian OPersian left a comment

Choose a reason for hiding this comment

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

👍

@dorosh dorosh merged commit 36fbadc into dev Feb 13, 2017
@z4y4ts z4y4ts deleted the bug/relocate-django-vars branch February 13, 2017 16:24
OPersian pushed a commit that referenced this pull request Feb 14, 2017
* Move django vars from js

-add valid json into player context on backend
-parse json on frontend and use js object as global var
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