-
Notifications
You must be signed in to change notification settings - Fork 317
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
Assignment toolbar #1
Conversation
@jhamrick it looks like you're not a collaborator on this repo. I've just made an nbgrader team and invited you to join it. You may decide to work through pull requests anyway - that's up to you and @ellisonbg - but it seems right that you should have full access to this repo. |
Awesome, thanks! 😃 |
Agreed, Jess should at least be admin and have right to add people to nbgrader team. In roughly two days I should be able to help again. For now last rehearsal... :-P
|
Great, I will review this today! |
Here is the metadata @jhamrick and I talked about this celltoolbar setting:
|
One other metadata field:
|
Ok, this has been update and is ready for review again. |
* Add a display class to the cell element, depending on the | ||
* nbgrader cell type. | ||
*/ | ||
var display_cell_type = function (cell) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because cells already have a cell_type
attribute in the document format, I find that using that name here is a bit confusing. Originally I was expecting to have two separate boolean attributes solution
and grade
. Trying to think if you would ever want a cell that was both solution and grade. Actually I think there is one. If you asked the students to write the tests themselves. Then you would want both to stub the cell in the student version, but still autograde it later. Based on this, I think we should use metadata.nbgrader.solution=true/false
and metadata.nbgrader.grade=true|false
. Then the UI for each would be a checkbox. Does this make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes, good point -- I'll change it to be this way. I think I must have been too tired when I was working on this last night as I totally missed that you had it that way in your comment!
Ok, I think I addressed all your comments. I went with |
One thing I realized though is that having the Perhaps we could do something where if there is the comment syntax, then only the subregion of the cell is replaced -- even if |
Hmm, this is actually reeeeallly slow to load for long notebooks (I have one notebook for which it actually times out). This seems problematic enough that I want to fiddle with it a bit before this is merged. |
Ok, done fiddling. The issue was in how I was loading it in my custom.js; I've modified the instructions at the top of nbgrader.js accordingly. |
Build is failing here because .travis.yml doesn't exist yet. |
} else { | ||
return cell.metadata.nbgrader.grade; | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be usefull : https://github.com/letsgetrandy/brototype#features :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh. Yes, but it would only be used in two places so I'm not sure I want to include a dependency just for that.
Looks good, a few suggestions inline. |
You mean the |
Yep, that was a comment for the future :-) I should have added a |
Ah, ok :-) And yeah, bootstrap might be a good idea given that ipython is already using it anyway. I'll play with it a bit... |
2fc96c9
to
5cb83c8
Compare
Seems like bootstrap doesn't play all that nicely with how the celltoolbar is styled by default, so I'm going to leave it for now. |
link.type = 'text/css'; | ||
link.rel = 'stylesheet'; | ||
link.href = require.toUrl('./nbgrader.css'); | ||
console.log(link); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!
@ellisonbg I think this is ready for merge -- do you want to do another review of this or should I just go ahead and merge it? |
text.addClass('nbgrader-id-input'); | ||
text.attr("value", cell.metadata.nbgrader.grader_id); | ||
text.keyup(function () { | ||
cell.metadata.nbgrader.grader_id = text.val(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been using a grader_id
in the context of peer grading, where there it is a uuid assigned to a person who is peer grading. Maybe grade_id
instead in this context?
OK, digging out of everything now...this looks awesome! I just tried it out and it works really well. Other than the one comment I just made about |
I'll go ahead and change it now, it's not too much trouble to do that. |
thanks, great work! On Wed, Oct 8, 2014 at 6:27 PM, Jessica B. Hamrick <notifications@github.com
Brian E. Granger |
Changing environment for validation
Add javascript and css for the nbgrader toolbar for creating assignments.