feat: scoreboard top, bottom, teacher, student #1272
Conversation
…y/rapid-router into scoreboard_bottom
…y/rapid-router into scoreboard_bottom
There was a problem hiding this comment.
Reviewed 4 of 10 files at r1, 1 of 6 files at r3, 1 of 4 files at r5, 3 of 5 files at r6, 5 of 5 files at r8, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @dionizh)
game/static/game/js/scoreboard.js, line 15 at r8 (raw file):
$('#select-all-levels').on('click', () => { $('[id^="id_episodes_"]').prop('checked', $('#select-all-levels').is(':checked')) })
empty line missing
game/templates/game/base.html, line 36 at r8 (raw file):
<div class="sub-nav sub-nav--teacher"> {% elif user|is_independent_student %} <div class="sub-nav" style="background-color: #f6be00">
Should we make something like a sub-nav--independent-student class in portal for this? 😬
game/templates/game/scoreboard.html, line 267 at r8 (raw file):
{{ student.total_score }}</td> </tr> {% else %}
Hm, is this else block almost an exact copy of the above except all tds have current-student? If so, should we move this class to the tr and shrink the ifelse to that tag only?
game/templates/game/scoreboard.html, line 317 at r8 (raw file):
{% endfor %} </table> {% endif %}
Should we add a "no data" message if the table is empty/doesn't exist?
game/templatetags/game/utils.py, line 52 at r8 (raw file):
and u.userprofile.student and u.userprofile.student.is_independent() )
Is there no way to use all of these directly from portal to avoid copying them? We may need to move them to common to share them.
game/views/scoreboard.py, line 126 at r8 (raw file):
class StudentInTrouble:
lol
game/views/scoreboard.py, line 168 at r8 (raw file):
all_students = students_of_classes(classes) the_students = [] # that need improvement
Minor, students_in_trouble? 😆
game/views/scoreboard.py, line 213 at r8 (raw file):
episode_ids = set(map(int, request.POST.getlist("episodes"))) # Get default data if sets above are empty. Also allows to load scoreboard using
This may need to be a frontend default, but not too sure. As far as I remember, the users can de-select the default first class and see an empty table if they choose to, though it would be a bit weird 🤔 It can also be confusing at the moment though - if they select nothing, but they still see results in the table.
game/views/scoreboard_csv.py, line 23 at r8 (raw file):
idx = f"{student.class_field.name}_{student.name}" improvement_dict[idx] = student.areas print(improvement_dict)
This print can be removed 🙂
There was a problem hiding this comment.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @dionizh)
game/templates/game/scoreboard.html, line 18 at r8 (raw file):
<script src="{% static 'game/js/scoreboard.js' %}"></script> <script> $(document).ready(function() {
I just noticed - since we have a scoreboard.js, shall we move this whole script there?
There was a problem hiding this comment.
Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @dionizh)
There was a problem hiding this comment.
Reviewable status: 12 of 14 files reviewed, 9 unresolved discussions (waiting on @razvan-pro)
game/templates/game/base.html, line 36 at r8 (raw file):
Previously, razvan-pro (Razvan Mahu) wrote…
Should we make something like a
sub-nav--independent-studentclass in portal for this? 😬
Ideally I think these should be available on portal CSS as RR does not have a base CSS for the scoreboard, level selection, etc. Thought maybe we can clean up later when restructuring with react 😬
game/templates/game/scoreboard.html, line 18 at r8 (raw file):
Previously, razvan-pro (Razvan Mahu) wrote…
I just noticed - since we have a
scoreboard.js, shall we move this whole script there?
Done.
game/templates/game/scoreboard.html, line 267 at r8 (raw file):
Previously, razvan-pro (Razvan Mahu) wrote…
Hm, is this else block almost an exact copy of the above except all
tds havecurrent-student? If so, should we move this class to thetrand shrink the ifelse to that tag only?
Ok moved things around.
game/templates/game/scoreboard.html, line 317 at r8 (raw file):
Previously, razvan-pro (Razvan Mahu) wrote…
Should we add a "no data" message if the table is empty/doesn't exist?
Mm there's already a no data message as I remember it. Probably comes from datatables?
game/templatetags/game/utils.py, line 52 at r8 (raw file):
Previously, razvan-pro (Razvan Mahu) wrote…
Is there no way to use all of these directly from portal to avoid copying them? We may need to move them to
commonto share them.
It was complaining without this bunch. Need another look.
game/views/scoreboard.py, line 126 at r8 (raw file):
Previously, razvan-pro (Razvan Mahu) wrote…
lol
😆
game/views/scoreboard.py, line 168 at r8 (raw file):
Previously, razvan-pro (Razvan Mahu) wrote…
Minor,
students_in_trouble? 😆
Better not overuse it 😂
game/views/scoreboard.py, line 213 at r8 (raw file):
Previously, razvan-pro (Razvan Mahu) wrote…
This may need to be a frontend default, but not too sure. As far as I remember, the users can de-select the default first class and see an empty table if they choose to, though it would be a bit weird 🤔 It can also be confusing at the moment though - if they select nothing, but they still see results in the table.
Need a closer look.
game/views/scoreboard_csv.py, line 23 at r8 (raw file):
Previously, razvan-pro (Razvan Mahu) wrote…
This
Oops, thanks!
There was a problem hiding this comment.
Reviewed 6 of 6 files at r11, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dionizh and @razvan-pro)
game/static/game/js/scoreboard.js, line 17 at r11 (raw file):
}); $(document).ready(function () {
Minor, we can probably put the above inside the ready function too
game/templates/game/base.html, line 36 at r8 (raw file):
Previously, dionizh (Dioni Zhong) wrote…
Ideally I think these should be available on portal CSS as RR does not have a base CSS for the scoreboard, level selection, etc. Thought maybe we can clean up later when restructuring with react 😬
Yep, in portal I was suggesting 🙂 Fine to do it later though 👍
game/templates/game/scoreboard.html, line 317 at r8 (raw file):
Previously, dionizh (Dioni Zhong) wrote…
Mm there's already a no data message as I remember it. Probably comes from datatables?
It shows like this to me 😬
game/tests/test_scoreboard.py, line 36 at r11 (raw file):
# print(student_row.level_scores) print(student_row.completed) print(student_row.percentage_complete)
Lines above to be removed I assume 😄
game/views/scoreboard.py, line 213 at r8 (raw file):
Previously, dionizh (Dioni Zhong) wrote…
Need a closer look.
Yep, I'll keep this open as a reminder, but feel free to dismiss it of course, if we decide to go with it like this 🙂
game/views/scoreboard_csv.py, line 23 at r8 (raw file):
Previously, dionizh (Dioni Zhong) wrote…
Oops, thanks!
Still here 😁
There was a problem hiding this comment.
Reviewed 8 of 8 files at r12, all commit messages.
Dismissed @razvan-pro from 3 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dionizh and @razvan-pro)
game/tests/test_scoreboard.py, line 36 at r11 (raw file):
Previously, razvan-pro (Razvan Mahu) wrote…
Lines above to be removed I assume 😄
Do we not need this function at all anymore?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dionizh, @faucomte97, and @razvan-pro)
game/static/game/js/scoreboard.js, line 17 at r11 (raw file):
Previously, razvan-pro (Razvan Mahu) wrote…
Minor, we can probably put the above inside the ready function too
Done.
game/templates/game/base.html, line 36 at r8 (raw file):
Previously, razvan-pro (Razvan Mahu) wrote…
Yep, in portal I was suggesting 🙂 Fine to do it later though 👍
Done.
Code quote:
sub-nav--studentgame/templates/game/scoreboard.html, line 317 at r8 (raw file):
Previously, razvan-pro (Razvan Mahu) wrote…
It shows like this to me 😬
Done.
game/templatetags/game/utils.py, line 52 at r8 (raw file):
Previously, dionizh (Dioni Zhong) wrote…
It was complaining without this bunch. Need another look.
Done, just need to load app_tags on scoreboard.html :)
game/tests/test_scoreboard.py, line 36 at r11 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
Do we not need this function at all anymore?
No it's not used anymore.
game/views/scoreboard_csv.py, line 23 at r8 (raw file):
Previously, razvan-pro (Razvan Mahu) wrote…
Still here 😁
Done now?
There was a problem hiding this comment.
Dismissed @razvan-pro from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dionizh)
There was a problem hiding this comment.
Reviewable status: 13 of 16 files reviewed, 4 unresolved discussions (waiting on @dionizh, @faucomte97, and @razvan-pro)
game/static/game/js/scoreboard.js, line 48 at r17 (raw file):
}); // document.getElementById("tableWrapper").scrollIntoView();
To remove?
game/views/scoreboard.py, line 213 at r17 (raw file):
episode_ids = set(map(int, request.POST.getlist("episodes"))) else: class_ids = {users_classes[0].id}
Can you get the default magic numbers to constants? Also add comment about defaulting to these values (essentially the comment you removed 😆)
game/views/scoreboard.py, line 231 at r17 (raw file):
classes=users_classes, initial={"classes": ["1"], "episodes": ["1"]}, )
Same here to constants.
There was a problem hiding this comment.
Reviewable status: 13 of 16 files reviewed, 4 unresolved discussions (waiting on @dionizh, @faucomte97, and @razvan-pro)
game/static/game/js/scoreboard.js, line 48 at r17 (raw file):
Previously, dionizh (Dioni Zhong) wrote…
To remove?
Done.
game/views/scoreboard.py, line 213 at r8 (raw file):
Previously, razvan-pro (Razvan Mahu) wrote…
Yep, I'll keep this open as a reminder, but feel free to dismiss it of course, if we decide to go with it like this 🙂
Done.
game/views/scoreboard.py, line 213 at r17 (raw file):
Previously, dionizh (Dioni Zhong) wrote…
Can you get the default magic numbers to constants? Also add comment about defaulting to these values (essentially the comment you removed 😆)
Done.
game/views/scoreboard.py, line 231 at r17 (raw file):
Previously, dionizh (Dioni Zhong) wrote…
Same here to constants.
Done.
There was a problem hiding this comment.
Dismissed @razvan-pro from a discussion.
Reviewable status: 13 of 16 files reviewed, all discussions resolved (waiting on @faucomte97 and @razvan-pro)
game/views/scoreboard.py, line 213 at r8 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
Done.
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r17, all commit messages.
Reviewable status: 14 of 16 files reviewed, all discussions resolved (waiting on @faucomte97 and @razvan-pro)
There was a problem hiding this comment.
Reviewed 2 of 2 files at r18.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @dionizh)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r19, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dionizh)
game/tests/test_scoreboard.py, line 205 at r19 (raw file):
# check first row class_name, name, completed_levels, total_time, total_scores, l1, l2, improvement = actual_rows[0].split(",")
Whitespaces 😛 I'd say run Black here since you have some long lines
There was a problem hiding this comment.
Reviewable status: 14 of 16 files reviewed, 1 unresolved discussion (waiting on @faucomte97 and @razvan-pro)
game/tests/test_scoreboard.py, line 205 at r19 (raw file):
Previously, faucomte97 (Florian Aucomte) wrote…
Whitespaces 😛 I'd say run Black here since you have some long lines
Oops, sorry forgot to turn the reformat back on after HTML work.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r20, all commit messages.
Reviewable status:
Codecov Report
@@ Coverage Diff @@
## master #1272 +/- ##
==========================================
+ Coverage 90.19% 90.64% +0.45%
==========================================
Files 98 98
Lines 5352 5315 -37
==========================================
- Hits 4827 4818 -9
+ Misses 525 497 -28
|
faucomte97
dismissed
razvan-pro’s stale review
Reviewed everything including Razvan's comments
Including sub nav bar for all types of users.
This change is