Skip to content

Replaced Rails loop with JavaScript map to handle instructor merges #26

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.

Sign up for GitHub

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

Open
jasonkhoe wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Replaced Rails loop with JavaScript map to handle instructor merges #26

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
28101d8
Replaced Rails loop with JavaScript map to handle instructor merges
jasonkhoe Mar 13, 2013
67787c8
Tidied up merge_instructors JavaScript
jasonkhoe Mar 21, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 13 additions & 12 deletions app/views/coursesurveys/merge_instructors.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,22 @@
profname = function(prof) { return prof.name; }
$(document).ready( function() {
$.ajax({ url: <%= raw coursesurveys_instructor_ids_path.inspect %>,
success: function(data) {
<%-[0,1].each do |i|-%>
$('#ibox<%=i%>').
autocomplete(data, {formatItem: profname} ).
result( function(event,item) {
$('#id_<%=i%>')[0].value = item.id;
$('#ibox<%=i%>')[0].disabled = true;
});
$('#ibox<%=i%>').keypress(function(event){if(event.which == 13) event.preventDefault();});
<%-end-%> }
});
success: function(data) {
[0,1].map(function(i) {
Copy link
Copy Markdown
Member

@richardxia richardxia Mar 16, 2013

Choose a reason for hiding this comment

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

I believe a for loop is more conventional in Javascript.

Copy link
Copy Markdown
Contributor Author

@jasonkhoe jasonkhoe Mar 16, 2013

Choose a reason for hiding this comment

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

With a for loop, I ran into weird issues regarding JavaScript closures. That is, the counter variable of the for loop would be incremented again (i = 2), and then a subsequent $("#ibox1").autocomplete would attempt to update #id_2 and #ibox2, which don't exist.

Copy link
Copy Markdown
Member

@richardxia richardxia Mar 18, 2013

Choose a reason for hiding this comment

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

Can you give an example of what you were trying with a for loop?

Copy link
Copy Markdown
Contributor Author

@jasonkhoe jasonkhoe Mar 21, 2013

Choose a reason for hiding this comment

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

I believe my original attempt went something like this:

success: function(data) {
  for (var i = 0; i < 2; i++) {
    $('#ibox' + i).
      autocomplete(data, {formatItem: profname} ).
    result( function(event,item) {
      $('#id_' + i)[0].value = item.id;
      $('#ibox' + i)[0].disabled = true;
    });
    $('#ibox' + i).keypress(function(event){if(event.which == 13) event.preventDefault();});
  }
}

Copy link
Copy Markdown
Member

@richardxia richardxia Mar 21, 2013

Choose a reason for hiding this comment

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

I played around with this code in the console, and I see why you ran into the i = 2 problem. The variable i referenced from within the autocomplete event handler is bound to the i from the same scope as the for loop, but when the event handler is actually called, the value of i has changed. Can we change this so that we can look up the id and ibox elements given just the ibox element via this or event.target?

$('#ibox' + i).
autocomplete(data, {formatItem: profname} ).
result( function(event,item) {
$('#id_' + i)[0].value = item.id;
$('#ibox' + i)[0].disabled = true;
});
Copy link
Copy Markdown
Member

@richardxia richardxia Mar 16, 2013

Choose a reason for hiding this comment

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

Nitpicks about the indentation and lane breaks here:

  1. success should be lined up with url.
  2. Why is autocomplete on a separate line? It fits within 80 characters with the previous line.
  3. result should be indented more.
  4. Having three nested anonymous functions in the success handler is pretty confusing to read. I would create a new named function and just reference it here.
  5. There is inconsistent spacing around function arguments. I would personally prefer no spaces before or after parentheses and a space after every comma.

$('#ibox' + i).keypress(function(event){if(event.which == 13) event.preventDefault();});
Copy link
Copy Markdown
Member

@richardxia richardxia Mar 16, 2013

Choose a reason for hiding this comment

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

This is pretty difficult to read and I don't actually understand what it's doing. Also, are you sure you want preventDefault and not return false? See http://stackoverflow.com/a/1357151

});
}
});
$("#submit").click( function() {
});
Copy link
Copy Markdown
Member

@richardxia richardxia Mar 16, 2013

Choose a reason for hiding this comment

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

It's not part of your changes, but what is the point of this? If it serves no function, can you delete it?


});
});
</script>

<style type="text/css">
Expand Down