-
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(ui): added logout feature #133
Conversation
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 would recommend you to setup ESlint and prettier in your editor.
You can also run npm run lint:fix
or npm run format
to fix these linting errors.
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.
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.
Add tests for this.
import styles from './styles.module.css'; | ||
|
||
export const Sidebar: React.FC<{}> = () => { | ||
const navigate = useNavigate(); | ||
const logoutHandler = () => { | ||
window.sessionStorage.removeItem('token'); |
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.
Logout is a multi step process. Following should be done in order:
- When user clicks on logout, a
DELETE
request should be sent to the API at/auth/token:revoke
- While the request is being processed, the loading should be shown and the whole screen should be disabled.
- Once the request succeeds, remove the token from
sessionStorage
and navigate user to/login
const navigate = useNavigate(); | ||
const logoutHandler = () => { | ||
window.sessionStorage.removeItem('token'); | ||
navigate('/login', {replace: true}); |
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 can omit replace
option.
replace: true
option replaces the current entry in url history stack with the one we are navigating to. We want current URL to exist in the history.
// isActive: boolean; | ||
} | ||
|
||
export const SidebarRow: React.FC<Props> = props => { | ||
return ( | ||
<div className={styles.container_row}> | ||
<div className={styles.container_row} onClick={props.onClick}> |
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 throw an error when onClick is not provided by the parent.
Something like this should make sure that we don't call onClick
when it's null.
<div className={styles.container_row} onClick = {() => props.onClick && props.onClick()}>
@prajjwalkapoor, please comment on #128 so that it can be assigned to you. |
@prajjwalkapoor, are you still working on this. It has been idle for some time. |
closes #128