-
Notifications
You must be signed in to change notification settings - Fork 2
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
[Re-add bars for professor-side poll results] #356
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chalo2000
requested review from
CameronHamidi,
kevinchan159,
matthewcoufal,
jackthmp and
YKo20010
February 10, 2020 03:34
YKo20010
reviewed
Feb 10, 2020
jackthmp
requested changes
Feb 24, 2020
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.
Nice work! just a couple minor things
jackthmp
reviewed
Feb 27, 2020
jackthmp
reviewed
Feb 27, 2020
jackthmp
approved these changes
Mar 4, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
The professor bars weren't proportional like the student ones, so I added that back.
Professor side is now also redesigned
Changes Made
I moved some things in and out of a switch statement to give the professor the students' proportional bars. There are, however, other issues to resolve before integrating this into the app.
Image 1)
Just re-enabling bars doesn't look very nice; the corner radius on the highlightView bar clashes with the rectangular outer bar of the containerView. The background colors of the behind the bars also make the correct answer bar blend in too much and the light gray bars clash against the darker background.
Image 2)
This is after I fixed the issues from above. It looks nicer now.
Image 3)
The issue with this approach is that without the green background, I cannot tell what was originally the right answer unless at least one person answers it. In this case, it is obvious that the first option is the correct answer since the second option is not green, but what would we do in cases of more than 2 choices to choose from and at least 2 choices with no answers at all?
I feel we need to get some designs to fix this.
Lots of refactoring in MCResultCell to make code more succinct and clearer.
Test Coverage
Manual testing (see images)
Next Steps
After figuring out the proper approach to resolve the new design issue, resolve more Github issues.
A couple of bugs discovered when implementing the new designs, especially for making draft questions (like #363). Will be putting up new issues to record these.
Related PRs or Issues
#344
Screenshots
Old screenshots
1 Just re-enabling bars
2 Modifying the bars a little
3 Issue with new modification
Student gets right answer
Student gets wrong answer
Professor side with 2 devices changing their answers