-
Notifications
You must be signed in to change notification settings - Fork 3
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
Less intrusive anon modal #495
Conversation
[diff-counting] Significant lines: 94. |
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.
This looks great, Matt! Left a few comments about styling and code style, but otherwise it worked well on my end :)
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.
Great work Matthew! This looks a lot more intuitive on the user side.
One thing about the emoji used--could you add it to the codebase as an svg instead to maintain consistency? There should be examples of this within Cornellians Say and Similar Courses features already!
setNoReviews(res.result === 0); | ||
} | ||
} | ||
|
||
function onProfessorChange(newSelectedProfessors: string[]) { |
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.
This can be part of a codebase cleanup PR, but it'd be nicer if these functions were all arrow functions (i.e. const onProfessorChange = (newSelectedProfessors: string[]) => {...}
).
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 only converted the four onXChange methods -- should I convert the other functions as well?
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.
Yeah, that would be great!
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 all the changes, Matt. Looks great!
Summary
This PR works on streamlining the semi-intrusive anon modal.
PR Type
Mobile + Desktop Screenshots & Recordings
QA - Test Plan
Tested on a variety of sizes and with logging in/out repeatedly.
Breaking Changes & Notes
Kept the modal around but changed its name from AnonymousWarning to LoginModal. Its only purpose is now to tell the user to log in after submitting a review without first having done so. All uses of the class should have been modified. I'm using Jaeyoung's adorable bear emoji for this class, if that's ok!
Now, there's text above the submit button informing users of their anonymity. Unlike the modal, this notice is always present, no matter how many reviews the user has written.
Added to documentation?
What GIF represents this PR?
gif