-
Notifications
You must be signed in to change notification settings - Fork 36
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
Add office filter to leaderboard #48
base: master
Are you sure you want to change the base?
Add office filter to leaderboard #48
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.
Thanks for your pull request.
Overall this looks great. Just added a question regarding org_title and a suggestion to DRY up the if statement for the leaderboard.
Any chance you could add a screenshot of the UI changes?
selected_dept=department, | ||
selected_timespan=timespan, | ||
selected_timespan=timespan, selected_office=office, | ||
org_title=config.ORG_TITLE, |
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 wonder if org_title is still being used? If not it should be removed here and in the config.
@@ -32,6 +32,15 @@ <h1>Lovers of the Week</h1> | |||
{% endfor %} | |||
</select> | |||
</div> | |||
<div class="form-group"> | |||
<label class="sr-only" for="office">Department</label> |
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 assume this should be Office not Department?
lovers.append((employee_key, c.sent_count)) | ||
else: | ||
employee = employee_key.get() | ||
if ( |
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.
Not blocking but I think this could need a comment saying what condition we are looking for to append an employee to the list.
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.
Maybe even better to extract this into a function with a little docstring to keep things DRY. Thoughts?
lovees.append((employee_key, c.received_count)) | ||
else: | ||
employee = employee_key.get() | ||
if ( |
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.
Not blocking but I think this could need a comment saying what condition we are looking for to append en employee to the list.
Add office filter to leaderboard