Skip to content

Add logout button - closes #1653 #1686

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.

Sign up for GitHub

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Add logout button - closes #1653 #1686

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e939f7d
add function for logout and to check login status
trezp Mar 22, 2023
8e3a16c
Add button to header
trezp Mar 22, 2023
0543bce
Merge branch 'master' into add-logout-button
trezp Mar 22, 2023
c0458d3
run prettier
trezp Mar 24, 2023
6ea82d8
Merge branch 'add-logout-button' of https://github.com/trezp/recordex…
trezp Mar 24, 2023
cdc9f22
Merge branch 'master' into add-logout-button
trezp Mar 24, 2023
219af29
minor formatting fix
trezp Mar 24, 2023
fbd9f02
Merge branch 'add-logout-button' of https://github.com/trezp/recordex…
trezp Mar 24, 2023
9501855
add auth state redux and update relevent files
trezp Mar 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 47 additions & 32 deletions src/frontend/src/components/Header/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,51 @@
import React from "react";
import { Link } from "react-router-dom";
import React, {useState, useEffect} from "react";
import { Link, useLocation } from "react-router-dom";
import Logo from "../Logo";
import { isLoggedIn, oeciLogout } from "../../service/cookie-service";

export default class Header extends React.Component {
public render() {
return (
<div className="bg-white shadow">
<nav
className="mw8 relative center flex flex-wrap justify-between pa3"
aria-label="Primary"
>
<div className="logo mb4 mb0-ns">
<Link to="/" aria-label="Home">
<Logo />
</Link>
</div>
<div className="mt5 mt2-ns">
<Link
to="/manual"
className="link hover-blue f5 fw6 pv2 ph0 ph3-ns mr4-ns"
>
Manual
</Link>
<Link
to="/record-search"
className="absolute top-1 right-1 static-ns bg-blue white bg-animate hover-bg-dark-blue f5 fw6 br2 pv2 ph3"

export default function Header (){
const [loggedIn, setLoggedIn] = useState(isLoggedIn());
const location = useLocation();

useEffect(() => {
setLoggedIn(isLoggedIn());
}, [location]);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any chance we could refactor this to use something like https://www.npmjs.com/package/react-cookie? It feels a bit odd to hook into location for this kind of thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KentShikama Can you tell me more about why this bothers you and what you would prefer to be done with this package?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KentShikama react-cookie wasn't going to work because it doesn't trigger a re-render when the cookies change, but I pushed another solution. Looks like I need to update so front end tests are passing though. Will re-request a review once I button that up.

Copy link
Member

@KentShikama KentShikama May 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trezp Pardon the delay in reply. It is just that the use of the location is "indirect" and anything indirect when it doesn't need to be adds complexity. Ideally, we would be reacting to when the cookie is added/removed/expired as that is what the status of the login button should reflect.

Copy link
Member

@KentShikama KentShikama May 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trezp

react-cookie wasn't going to work because it doesn't trigger a re-render when the cookies change

Ah ok. Pardon for taking you down that path then.

return (
<div className="bg-white shadow">
<nav
className="mw8 relative center flex flex-wrap justify-between pa3"
aria-label="Primary"
>
<div className="logo mb4 mb0-ns">
<Link to="/" aria-label="Home">
<Logo />
</Link>
</div>
<div className="mt5 mt2-ns">
<Link
to="/manual"
className="link hover-blue f5 fw6 pv2 ph0 ph3-ns mr4-ns"
>
Manual
</Link>
<Link
to="/record-search"
className="absolute top-1 right-1 static-ns bg-blue white bg-animate hover-bg-dark-blue f5 fw6 br2 pv2 ph3"
>
Search
</Link>
{loggedIn && (
<button
onClick={oeciLogout}
className="absolute top-1 left-2 static-ns bg-white f5 fw6 br2 ba b--blue blue link hover-dark-blue pv2 ph3 ml2"
>
Search
</Link>
</div>
</nav>
</div>
);
}
Log Out
</button>
)}
</div>
</nav>
</div>
);
}
12 changes: 12 additions & 0 deletions src/frontend/src/service/cookie-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import history from "../service/history";
import store, {clearAllData} from "../redux/store";

interface Cookie {
[key: string]: string;
Expand Down Expand Up @@ -38,3 +39,14 @@ export function checkOeciRedirect() {
history.replace("/oeci");
}
}

export function isLoggedIn() {
return hasOeciToken();
}

export function oeciLogout() {
removeCookie();
store.dispatch(clearAllData());
history.replace("/oeci");
}