-
Notifications
You must be signed in to change notification settings - Fork 90
Adds popup notif. (fixes #1183) #1343
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
base: main
Are you sure you want to change the base?
Conversation
alexespejo
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.
Congrats on your first PR!!
This looks like a solid start! This is a smaller change so it's not too big but in the future if you can provide screen shots in the PR that would be good 🙂
When it comes to user signin notifications we typically use dialogs or popups like this to inform them:

We create an AlertDialog component for this particular instance
|
Summary Test Plan
Closes #1183 |
| open={isSignoutDialogOpen} | ||
| title={`Signed out of ${accountName}`} | ||
| severity="info" | ||
| onClose={async () => { |
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.
We want the dialog to appear after the user has signed out. With the current logic the user has to click the "Confirm" button on this modal to actually log out (i.e if the user refreshes the page before they click "Confirm" they'll still be logged in).
I would look into the logic we use to determine if a user is logged in. You can also make use of local storage to save whether we're in a "logout" state in order to display the message.
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.
In the LocalStorageKeys, added signoutNotice flag and uses it for proper behavior change: Clicking "Log out" immediately clears session and sets signoutNotice.
| open={isSignoutDialogOpen} | ||
| title={`Signed out of ${accountName}`} | ||
| severity="info" | ||
| onClose={async () => { |
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.
Aside: when we bind functions to event handlers like this we typically define a separate function in the body of the component just to create a separation of concerns :)
function onClick(){}
...
<AlertDialog>
onClose={onClick}
</AlertDialog>
| </Menu> | ||
| <AlertDialog | ||
| open={isSignoutDialogOpen} | ||
| title={`Signed out of ${accountName}`} |
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.
ed2dc4c to
edb48d7
Compare
alexespejo
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.
My fault, I gave bad instructions that are too overkill.
Revert the commits related to the suggested change.
The flow should be:
"Hit logout" -> invalidates session -> display alert -> "user hits close" -> reload the page
*the current issue is if you hit logout the user HAS to hit "close" in order to be properly invalidated without being dependent on the user input
edb48d7 to
67cde9e
Compare
| export function removeLocalStorageSessionId() { | ||
| window.localStorage.removeItem(LSK.sessionId); | ||
| } | ||
|
|
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.
fix: these changes aren't needed anymore, revert the file changes
alexespejo
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.
Resolve the merge conflicts so the deployment can run
4751641 to
67cde9e
Compare


Summary
Improves the sign-out flow by showing a notification when the user signs out of their Google Account.
This update uses the global openSnackbar action to display a top-right toast message including the user's name or email (when available).
The logout notification appears before the session is cleared to ensure it is always visible to the user.
Test Plan
Manually signed it with test account and clicked 'Sign out'.
Dropdown menu closes and Snackbar appear top right corner
Message shows one of three choices
After 800ms delay, session clears through clearSession() and redirects to /.
Issues
Closes #1183