-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add logout button to burger menu in mobile #321
base: master
Are you sure you want to change the base?
Add logout button to burger menu in mobile #321
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.
Is "logoutCallback" a misnomer since it is never passed as a callback function?
Ok @thkruz What name do you propose? |
@martinvarelaaaa was a genuine question. "closeAndLogout" sounds more intuitive to me if it isn't a callback, but I was mainly just concerned I misunderstood the code (react is a bit confusing to me). |
Done @thkruz ! |
@martinvarelaaaa Thanks for this Martín, ill review asap - probably tomorrow. |
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.
@martinvarelaaaa Great work Martín, and thanks for the input @thkruz
The code and functionality looks good to me, but I think we can make one small tweak to ensure that whenever the user "logs out" using the burger menu, they always get kicked back to the "home page." This is the user-experience that is currently in place when a user logs out via the AccountSettings
page, and it would be cool to ensure that this is replicated by the burger menu log out.
If you check AccountSettings
component you will see an import entitled withRouter
from react-router-dom
. This helper allows any component to avail of the routers history
object - so that a push to a different route can occur within the component. To use history
within the component you have to pass it to the withRouter
helper at the bottom of the file when exporting it.
Like so:
export default withRouter(UserSettings);
And then you can access it via the de-structuring syntax like so:
function UserSettings({ history }) {
...
}
Note - this would mean that your check for the history
helper inside the logoutAndClose
function would now be redundant - as both components would have access to the routers history.
If you can push those couple of tweaks, ill merge and deploy.
Hope my notes make sense to you. Any problems or questions just let me know.
Thanks!
John.
Ok perfect @johngribbin! I'll probably turn up the fix tomorrow |
Issue #281
logout
function is added inauth-helpers
to reuse the codeExecution:
https://drive.google.com/file/d/1gxiU00UBtjd-h2ktnGNybLqE1KLI9y8x/view