-
-
Notifications
You must be signed in to change notification settings - Fork 62
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
make logout from settings modal functional #241
Conversation
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
closes #242 |
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 👍
@@ -196,6 +197,14 @@ const Config: SettingsConfiguration<{ server: Server }> = { | |||
{ | |||
id: "logout", |
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.
Omit id, only onClick is necessary.
@@ -227,4 +236,5 @@ const ClientSettingsRouting: Record<string, Component> = { | |||
sync, | |||
native, | |||
experiments, | |||
logout, |
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.
Once id is omitted, this component no longer needs to be included.
href: "/login", | ||
onClick: () => { | ||
const acli = getController("state"); | ||
const cli = getController("client").getCurrentClient(); |
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.
client is available through context here, add const client = useClient()
at the top of the function instead of using this line here.
@@ -196,6 +197,14 @@ const Config: SettingsConfiguration<{ server: Server }> = { | |||
{ | |||
id: "logout", | |||
icon: <BiSolidExit size={20} color={theme!.colours.error} />, | |||
href: "/login", | |||
onClick: () => { | |||
const acli = getController("state"); |
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 package should also have direct access to state and it can be called through state.
onClick: () => { | ||
const acli = getController("state"); | ||
const cli = getController("client").getCurrentClient(); | ||
console.log(cli); |
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.
Extraneous print statement
const acli = getController("state"); | ||
const cli = getController("client").getCurrentClient(); | ||
console.log(cli); | ||
acli.auth.removeSession(cli?.user?.id as 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 will remove the session from storage but not kill the active client in the client controller, which is remedied by assigning window.location.href
(since that will just kill all active sessions) but we want to avoid reloading the client.
const cli = getController("client").getCurrentClient(); | ||
console.log(cli); | ||
acli.auth.removeSession(cli?.user?.id as string); | ||
window.location.href = "/login"; |
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.
Should useNavigate from routing package at top of component, and then use navigate function instead of telling the browser to redirect us.
PR is stale, requested changes ignored, closing out for now |
Please make sure to check the following tasks before opening and submitting a PR
closes #242