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

Added ability to upload transcripts for teachers #23

Closed
wants to merge 12 commits into from

Conversation

sendr
Copy link

@sendr sendr commented Dec 8, 2016

No description provided.

@@ -1,5 +1,5 @@
{% load i18n %}
<div class="editor-with-buttons">
<div class="editor-with-buttons xmodule_VideoDescriptor">
Copy link

Choose a reason for hiding this comment

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

why do we need this class?

Copy link
Author

Choose a reason for hiding this comment

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

to use built-in styles

Copy link

Choose a reason for hiding this comment

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

to use built-in styles

Try avoid dependencies with edX. XBlock should be independent as much as possible.

@@ -0,0 +1,10 @@

.wrapper-translations-settings{
Copy link

Choose a reason for hiding this comment

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

and a whitespace befor {

@@ -14,6 +15,7 @@
from xblockutils.studio_editable import StudioEditableXBlockMixin

from django.template import Template, Context
from django.conf import settings
Copy link

Choose a reason for hiding this comment

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

try avoid using django settings

scope=Scope.content,
display_name=_('Upload transcript'),
help=_('Add transcripts in different languages. '
'Click below to specify a language and upload an .srt transcript file for that language.')
Copy link

Choose a reason for hiding this comment

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

fix according edx code style

@polesye
Copy link

polesye commented Dec 8, 2016

it'd be great if you could setup a demo for me

@@ -14,7 +14,7 @@ <h3 class="hd hd-2">{{display_name}}</h3>
<ul class="wrapper-downloads-custom">
{% if handout %}
<li class="video-handout video-download-button-custom">
<a href="{{ handout }}" class="download-handout" download="{{ handout_file_name }}">{% trans 'Download Handout' %}</a>
<a href="/{{ handout }}" class="download-handout" download="{{ handout_file_name }}">{% trans 'Download Handout' %}</a>
Copy link

Choose a reason for hiding this comment

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

Why don't do it in python?

<input class="input-file-uploader" name="file" type="file" data-change-field-name="">
</form>

<form method="POST" action="/assets/{{ courseKey }}/" class="transcript-uploader-form is-hidden" enctype="multipart/form-data">
Copy link

Choose a reason for hiding this comment

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

why do we need 2 forms? can we use just one?

var langExists = false;
for (var i=0; i < languages.length; i++){
if (lang == languages[i].lang){
langExists = true
Copy link

Choose a reason for hiding this comment

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

do we need to break ?

event.preventDefault();

var $choiserItem = $('.list-settings-item:hidden').clone();
$choiserItem.removeClass('hidden');
Copy link

Choose a reason for hiding this comment

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

you should use is-hidden instead of hidden. See https://smacss.com/book/type-state

fragment.add_css(self.resource_string("static/css/transcripts.css"))
fragment.add_css(self.render_resource("static/css/studio-main-v1.css",
path_to_images=path_to_images,
path_to_fonts=path_to_fonts))
Copy link

Choose a reason for hiding this comment

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

fix alignment


def get_url_for(self, field):
"""
It return url for download of file, which is stored in db
Copy link

Choose a reason for hiding this comment

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

fix typos

@polesye
Copy link

polesye commented Dec 12, 2016

  1. How does it handle trascript removing?
  2. Replace - Download buttons, not Upload - Download
  3. How do you handle parallel uploading?
  4. add file format validation

@@ -9,7 +9,7 @@
data-default="{% if field.type == 'boolean' %}{{ field.default|yesno:'1,0' }}{% else %}{{ field.default|default_if_none:"" }}{% endif %}"
data-cast="{{field.type}}"
>
<div class='wrapper-comp-setting {% if field.type == "set" %}metadata-list-enum{% endif %} {% if field.type == "file_uploader" %}file-uploader{% endif %}'>
<div class='wrapper-comp-setting {% if field.type == "set" %}metadata-list-enum{% endif %} {% if field.type == "file_uploader" %}file-uploader{% endif %}{% if field.type == "transcript_uploader" %}metadata-video-translations{% endif %}'>
Copy link

Choose a reason for hiding this comment

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

can we avoid such changes?

</select>
<a href="#" class="remove-action remove-setting" data-value=""><span class="icon fa fa-times-circle" aria-hidden="true"></span><span class="sr">Remove</span></a>
<div class="list-settings-buttons">
<a href="#" class="upload-setting upload-transcript is-hidden" data-change-field-name="{{field.name}}" data-lang-code="" data-lang-label="">{% trans "Upload" %}</a>
Copy link

Choose a reason for hiding this comment

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

remove field.name

var $fileUploader = $('.input-file-uploader', element);
var $langChoicer = $('.lang-choiser', element);
Copy link

Choose a reason for hiding this comment

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

typo

var indexLanguage;
for (var i=0; i < languages.length; i++){
if (lang == languages[i].lang){
indexLanguage = i
Copy link

Choose a reason for hiding this comment

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

break ?

'lang-code': $buttonBlock.data('lang-code'),
'lang-label': $buttonBlock.data('lang-label'),
'change-field-name': $buttonBlock.data('change-field-name')
});
Copy link

Choose a reason for hiding this comment

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

can be removed

$('.add-transcript', element).on('click', function (event) {
event.preventDefault();
$(event.currentTarget).addClass('is-disabled');
var $choiserItem = $('.list-settings-item:hidden').clone();
Copy link

Choose a reason for hiding this comment

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

choiser -> chosenItem

Copy link

Choose a reason for hiding this comment

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

templateItem

var lang = $currentBlock.find('option:selected').val();
for (var i=0; i < languages.length; i++){
if(lang == languages[i].lang){
languages.splice(i,1)
Copy link

Choose a reason for hiding this comment

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

Please find more appropriate name for languages. transcriptsValue

Copy link

Choose a reason for hiding this comment

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

Array.filter

e.preventDefault();
runtime.notify('cancel', {});
if(!languages.length){
$currentBlock.parents('li').removeClass('is-set');
Copy link

Choose a reason for hiding this comment

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

use chain

path_to_images=path_to_images,
path_to_fonts=path_to_fonts
)
)
Copy link

Choose a reason for hiding this comment

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

fix according to edX code style

Copy link

Choose a reason for hiding this comment

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


def get_url_for(self, field):
"""
It returns url for download of file, which is stored in db
Copy link

Choose a reason for hiding this comment

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

Returns downloaded asset url.

@z4y4ts z4y4ts closed this Dec 13, 2016
@z4y4ts z4y4ts reopened this Dec 13, 2016
@z4y4ts
Copy link

z4y4ts commented Dec 13, 2016

This code was merged in PR #25

@z4y4ts z4y4ts closed this Dec 13, 2016
@sendr sendr deleted the feature/transcript_upload branch February 24, 2017 15:38
dyudyunov pushed a commit that referenced this pull request Jan 13, 2022
Release v0.9.4: player controls fixes
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.

5 participants