Conversation
KentShikama
left a comment
KentShikama
left a comment
There was a problem hiding this comment.
Thanks for giving this another shot!
| useEffect(() => { | ||
| setLoggedIn(isLoggedIn()); | ||
| }, [location]); | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@KentShikama Can you tell me more about why this bothers you and what you would prefer to be done with this package?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
KentShikama
left a comment
KentShikama
left a comment
There was a problem hiding this comment.
Pardon. Didn't mean to approve.
2 participants
Closes #1653