-
Notifications
You must be signed in to change notification settings - Fork 4
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
Updates timeout handling to match requirements #333
Conversation
# Conflicts: # public/locales/en/common.json # public/locales/es/common.json
// handy converter for Minutes -> Milliseconds | ||
const ONE_MINUTE_MS = 60 * 1000 | ||
|
||
// keep times in human readable minutes | ||
const REDIRECT_TIMER = 30 | ||
const WARNING_DURATION = 5 | ||
const WARNING_TIMER = REDIRECT_TIMER - WARNING_DURATION |
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.
Converted these to human readable minutes, converting to milliseconds just in time
useEffect(() => { | ||
if (showWarningModal) { | ||
const timer = setTimeout(() => { | ||
setNumberOfMinutes(numberOfMinutes - 1) | ||
}, 60 * 1000) | ||
return () => clearTimeout(timer) | ||
} |
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 was just here twice. Oops 😅
} | ||
} | ||
|
||
function startOrUpdate() { |
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.
The big change in this PR is this is no longer a loop that resets, but instead we only run once. So no longer any need to clear and reset our base state, and instead we need to just track if we're before warning, warning, or done warning.
showWarningModal
tells us whether we're actively warning or not, and warned
tells us whether we've warned or not.
if (warningTimerId) { | ||
clearTimeout(warningTimerId) | ||
warningTimerId = null | ||
} |
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.
clear our all timers related to the going-to-warn.
function redirectToUIHome() { | ||
const uioHomeLink = userArrivedFromUioMobile ? getUrl('uio-home-url-mobile') : getUrl('uio-home-url-desktop') | ||
window.location.href = uioHomeLink || '' |
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.
Another change: instead of the button dismissing the modal, it lets the user head back to UI Home to keep their session active. This is only if they hit the button though - X'ing out the modal or clicking outside it lets them stay
public/locales/en/common.json
Outdated
"warning": "To protect your information, you will be logged out in {{count}} minute if you do not continue", | ||
"warning_plural": "To protect your information, you will be logged out in {{count}} minutes if you do not continue", | ||
"button": "Return to UI Home" |
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.
Updated verbiage matches Content doc - and properly handles the plural!
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.
@lomky I started to review this PR and then realized that my FE react knowledge just isn't quite strong enough to review TimeoutModal.tsx
yet. Could you walk me through it synchronously?
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, thanks for simplifying the code!
does timeoutmodal not have a snapshot or is it just not included in this PR? |
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.
@lomky Thanks for walking me through TimeoutModal.tsx
! This LGTM with a few minor changes:
- The modal sentence is missing a period.
- The modal bold is in the wrong font
When the user has been on the site for ~25 minutes an alert appears letting them know they will be logged out in 5 minutes. Previously the user could acknowledge this alert and reset the timer. Now the user may navigate back to UI Home to refresh their session, or they can dismiss the modal to stay on the page, but will be redirected to login again regardless in 5 minutes.
===
Resolves #199
main