Add office filter to leaderboard #48
Conversation
rockdog
left a comment
rockdog
left a comment
There was a problem hiding this comment.
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.
I wonder if org_title is still being used? If not it should be removed here and in the config.
| </select> | ||
| </div> | ||
| <div class="form-group"> | ||
| <label class="sr-only" for="office">Department</label> |
There was a problem hiding this comment.
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.
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.
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.
Not blocking but I think this could need a comment saying what condition we are looking for to append en employee to the list.
2 participants
Add office filter to leaderboard