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

April review fixes 146 #147

Merged
merged 11 commits into from
Jul 8, 2020
Merged

April review fixes 146 #147

merged 11 commits into from
Jul 8, 2020

Conversation

gibsonbailey
Copy link
Collaborator

@gibsonbailey gibsonbailey commented May 7, 2020

This PR is built to address the problems discovered in the April review of Chagrade on May 7th (Outlined in #146)

Checklist

  • Code review by me
  • Hand tested by me
  • I'm proud of my work
  • Code review by reviewer
  • Hand tested by reviewer
  • Ready to merge

…rol over ordering. Remove length constraint from definition and class descriptions. Adjust forms accordingly.
…ter.org's viewer rather than on Github. Fix formatting of grade detail page for custom_questions_only submissions and for submissions with no criteria, no custom questions, and no standard questions.
Copy link
Collaborator Author

@gibsonbailey gibsonbailey left a comment

Choose a reason for hiding this comment

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

Fixed reasonable issues that my eye could find.

@gibsonbailey gibsonbailey requested a review from ckcollab May 7, 2020 23:24
@@ -81,6 +91,9 @@ <h4>Questions</h4>
target="_blank" rel="noopener noreferrer"
>{{ submission.publication_url }}</a></li>{% endif %}
</ol>
{% if not definition.ask_project_url and not definition.ask_publication_url and not definition.ask_method_name and not definition.ask_method_description %}
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems kinda grody, could this become a model property or some such?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Very grody. Can do. Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -245,7 +252,12 @@ <h4 class="ui header">Submission Details:</h4>
{% if definition.jupyter_notebook_enabled %}
<div class="ui tiny statistic">
<div class="value">
{{ grade.jupyter_notebook_grade }}/{{ definition.jupyter_notebook_highest }}
{% if not grade.jupyter_notebook_grade %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be able to do this, may have to make 0.0 a string:

{{ grade.jupyter_notebook_grade|default:0.0 }}

https://docs.djangoproject.com/en/3.0/ref/templates/builtins/#default

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

✅ Good call

{% else %}
href="{{ submission.github_url }}"
{% endif %}
target="_blank" rel="noopener noreferrer">
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this would read a bit nicer if there was a property on the submission model that returned either github url or notebook url? in general moving logic out of templates is ideal, but can't always do it!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Model: big
Template: smol

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@gibsonbailey gibsonbailey temporarily deployed to chagrade-staging May 14, 2020 17:01 Inactive
@gibsonbailey gibsonbailey merged commit b2b5ca6 into develop Jul 8, 2020
@gibsonbailey gibsonbailey deleted the april-review-fixes-146 branch July 8, 2020 06:11
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.

2 participants