-
Notifications
You must be signed in to change notification settings - Fork 1
Support multiple problems on results and spectator pages #178
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
Conversation
|
This PR looks great, love that there is both a game overview table and tables for each problem. 😃
I think the UI is good! I have a couple potential ideas which I listed below, but honestly I think this design is good to merge, these are more post-MVP ideas in my mind:
I think this makes sense when scoring based on problems solved, I'll just mention that I think we may switch the overall scoring to calculate based on the percentage of test cases passed rather than the number of problems completely solved. In that case, I think a percentage would be wise because the number of test cases could be a strange amount like "17", where I think a rounded percentage makes more sense. In that case, it may be a bit strange to see the score style switch from a fraction to percentage in my opinion. Thoughts @alankbi? Either way, I think this is ok for MVP (are we still at that stage haha), so we could talk about further design changes later and keep this change if you prefer that. |
zpChris
left a comment
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! ✅
I left some general comments above this, but even without any of those changes I think this is good to merge.
I'll note that this logic is getting increasingly more complicated haha, which I suppose is bound to happen; IIUC though, this should be one of the last PRs for a while that works on the game page. Thanks for fixing the issues I mentioned in #174.
|
Thanks for those suggestions - I played around with them for a bit and wasn't able to fix it unfortunately, but was able to figure out thanks to those links that it's caused when switching between problems/sometimes when the code first loads due to the difference in number of characters between the different code states - will try to figure out a solution to this tomorrow. EDIT: it's fixed now :) that second link worked great, just needed to wrap it in a working useEffect
Yeah that's definitely something to consider and also something that I think teacher feedback will help us with a lot (what kind of system they prefer, whether the leaderboard should even exist, etc.) If we end up going that route then I agree percentages would probably make more sense - although another thing to consider as well would be the interpretability of the results (e.g. 2/3 solved might have more meaning to a teacher than say 34% passed when it comes to how well a student performed/learned).
Sounds good - yeah, for those other suggestions, I think I'll leave those for a future PR in that case (just to try to wrap up the MVP features quicker), but I agree on a lot of those (repositioning those buttons, a blinking dot of sorts, etc.)
Yeah I def got some OS class fork and exec vibes when working on PlayerGameView.tsx haha - basically how like the component is used both for players and spectators and like certain code blocks are only executed when you're one or the other lol |
Glad it worked, thanks so much! Yeah I think that removing that selection is more important than the teacher not being able to copy quickly when a student is typing quickly imo, so this solution works great by me. 👍
Those are all good points - sounds great! 👍
Agreed, let's get this features in! 😁
Haven't heard of that before, but yeah for sure, maybe at one point post-MVP we'll make that distinction clearer. Thanks for making all the changes! After you merge #174, this looks great to merge 🎉 🚀 |
List of Changes:
Notes:
2/5->3/5rather than40%->60%when correctly solving a problem - however, I'm open to feedback on this if there are any better options out thereWhen first entering spectate mode, it fetches theEDIT: I implemented the new solution, so now it should work without issues :)defaultCodeListwhich takes a short while - this also means that it initially will show only the latest submitted code for each problem until the player types something to update it. I felt like this was the most efficient option, although this is a clear downside, so I may look into alternatives (e.g. sending the full code list when first subscribing as mentioned in standup).Screencast and Images:
Screencast of new behavior
Results table game overview:
Results table specific problem:
