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

Update CAT create/edit view to support creation and modification of credit courses #299

Merged
merged 1 commit into from Aug 31, 2015

Conversation

rlucioni
Copy link
Contributor

XCOM-511

@clintonb and @AlasdairSwan, this is still a WIP, but I'd appreciate an early look, if you have the time. I'm still trying to get the table validation to work properly, and I still need to write tests.

viewClass = this.courseSeatViewMappings[seatType];

if (viewClass && model) {
if (viewClass && seats.length >= 1) {

Choose a reason for hiding this comment

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

Nitpick: > 0 would mean you can reduce the code by 1 character

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@rlucioni rlucioni force-pushed the renzo/credit-create-edit branch 11 times, most recently from 1a3650f to 21038ff Compare August 26, 2015 16:44
/* istanbul ignore next */
Backbone.Validation.bind(view, {
valid: function (view, attr) {
var $el = view.$('[name=' + attr + ']'),

Choose a reason for hiding this comment

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

Shouldn't this be

view.$el.find('[name=' + attr + ']'),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

_s.sprintf will help avoid bugs here.

course: this.course
});

row.render();

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, missed that. rowView no longer does that.

@rlucioni rlucioni force-pushed the renzo/credit-create-edit branch 2 times, most recently from 00673d2 to 2df855f Compare August 31, 2015 21:21
@rlucioni
Copy link
Contributor Author

@clintonb or @AlasdairSwan, any further comments? I've addressed everything up to this point.

@clintonb
Copy link
Contributor

👍

@rlucioni
Copy link
Contributor Author

@AlasdairSwan if you have further comments, let me know and I'll address them with a follow-up PR.

rlucioni pushed a commit that referenced this pull request Aug 31, 2015
Update CAT create/edit view to support creation and modification of credit courses
@rlucioni rlucioni merged commit 4fd2ac0 into master Aug 31, 2015
@rlucioni rlucioni deleted the renzo/credit-create-edit branch August 31, 2015 21:42
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.

None yet

3 participants