-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: new auth ui #1639
feat: new auth ui #1639
Conversation
Size Change: +278 B (0%) Total Size: 3.1 MB
ℹ️ View Unchanged
|
<Notice | ||
// Increment for each new notice (though you don't need to change it | ||
// when removing a notice), or users who previously dismissed the banner | ||
// won't see the updated content. | ||
key="unauthenticated-notice" | ||
id={5} | ||
> |
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.
Don't use a notice because it's closeable and then will never reopen. Use a separate dialog box in the lower right corner. See also #1633
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.
@benzuzu Thoughts on taking over? I'm kinda scuffed when it comes to frontend :)
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.
@Josh-Cena, I've thought about this more and I think we should replace the Me icon with a primary colored sign in button for unauthenticated access - something very similar to the approach google.com takes.
As for #1633, I think the exclamation point in the Me icon with an action item in the dropdown is the way to go.
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.
Yep agreed for the first part. See my previous slack message:
(For logged in users) I think it should show name initials instead of the default icon here, and we should have a way to let people customize their name in case there's no Yalies data, they have diff preferred name, etc.
But I also like your idea of a primary colored button.
Thanks for your pull request! The preview of your changes is available at https://public-banner.preview.coursetable.com. |
507a9e0
to
28c12bf
Compare
ef76603
to
3f2e6c5
Compare
@neilsong @Josh-Cena still viable? |
Yeah still viable. Not sure what exactly to do though. |
Closing as #1633 has been addressed and I think it's clear enough that users aren't logged in if they can't see ratings |
No description provided.