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

Fixed bugs #29

Merged
merged 3 commits into from
Dec 19, 2016
Merged

Fixed bugs #29

merged 3 commits into from
Dec 19, 2016

Conversation

sendr
Copy link

@sendr sendr commented Dec 16, 2016

  • 500 error with canceling of file upload
  • Handout download on both sides
  • Add status of upload handout
  • Add name of file to status of uploading
  • Add name of file to download attribute in button "Download"
  • After uploading handout replace wording "Upload" to "Replace" in button

 - 500 error with canceling of file upload
 - Handout download on both sides
 - Add status of upload handout
 - Add name of file to status of uploading
 - Add name of file to download attribute in button "Download"
 - After uploading handout replace wording "Upload" to "Replace" in button
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.

Looks good in general. Few things need to be addressed before merge.

var showUploadStatus = function($element, filename){
$element.find('.status-upload').text('File ' + '"' + filename + '"' + ' uploaded successfully').show();
setTimeout(function(){
$('.status-upload', $element).hide()
Copy link

Choose a reason for hiding this comment

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

Is this default behavior of edx video module?

} else {
if($fileUploader.val()){
$('.file-uploader-form', element).ajaxSubmit({
success: function(response, statusText, xhr, form) {
Copy link

Choose a reason for hiding this comment

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

Please extract success callback into a function for easier reading.

if(fieldName == "handout"){
$('input[data-field-name=' + fieldName + ']').val(response['asset']['id']).change();
} else {
if($fileUploader.val()){
Copy link

Choose a reason for hiding this comment

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

Please invert this if-clause to short-circuit in case if .val() is empty.
So that normal flow is outside if block.

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.

Improve successHanlder name, squash and merge.

Please be sure to write a good commit message.

@@ -309,42 +309,50 @@ function StudioEditableXBlock(runtime, element) {
}, 5000);
};

var successHandler = function(response, statusText, xhr, fieldName, lang, label, currentLiTag) {
Copy link

Choose a reason for hiding this comment

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

Please write more descriptive name, saying what this function does.

var url = '/' + response['asset']['id'];
var regExp = /.*@(.+\..+)/;
var filename = regExp.exec(url)[1];
if(fieldName == "handout"){
Copy link

Choose a reason for hiding this comment

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

Add after if

@@ -352,6 +374,10 @@ function StudioEditableXBlock(runtime, element) {

$('.setting-clear').on('click', function (event){
var $currentBlock = $(event.currentTarget).closest('li');
if($('.file-uploader', $currentBlock).length > 0){
Copy link

Choose a reason for hiding this comment

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

Add after if

@sendr sendr merged commit 37c18e6 into dev Dec 19, 2016
@sendr sendr deleted the bugfix/upload-download-handout branch February 24, 2017 15:29
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.

4 participants