-
Notifications
You must be signed in to change notification settings - Fork 41
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
#M27. Add "Status" model to replace "Last LMS module" field with a Foreignkey. #236
base: master
Are you sure you want to change the base?
#M27. Add "Status" model to replace "Last LMS module" field with a Foreignkey. #236
Conversation
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.
LGTM - a few comments and suggestions, but otherwise, nicely done to convert into statuses instead of LMS level. 👍🏻
"model": "accounts.status", | ||
"pk": 6, | ||
"fields": { | ||
"display_name": "Studying for a (between Project 4 & 5)", |
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.
Why is this the only fixture that states: Studying for a
whereas the others all just say Studying
?
"model": "accounts.status", | ||
"pk": 8, | ||
"fields": { | ||
"display_name": "Working as Developer 0-3 yrs", |
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 would make this 0-2 years
, and then pk9 can stay 3-5 years
so the 3rd year doesn't overlap.
accounts/fixtures/statuses.json
Outdated
"display_order": 13 | ||
} | ||
} | ||
] |
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.
EOF blank line missing
{% endfor %} | ||
</table> | ||
</div> | ||
|
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.
Outer div with class of table-responsive
seems to have gone missing... was this intentional, and is it still responsive?
<button class="btn btn-primary btn-sm float-right downloadTable" data-tableid="judgesTable">Export Judges Data to CSV</button> | ||
</div> | ||
|
||
<table class="table table-sm border-top-0" id="judgesTable"> |
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.
Same here... outer div with table-responsive
is now missing... assuming this was intentional, and does it still respond well?
8dc853c
to
1b26604
Compare
Description
Pull request type
Related Issue
#230
Configuration instructions
Testing
All tests passing successfully.