-
Notifications
You must be signed in to change notification settings - Fork 188
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
Fix lag banner #1741
Fix lag banner #1741
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.
Mostly just comments on how your variable names could be more descriptive. As we discussed in person, you want to think about the context of where your variables are defined when deciding how specific variable names should be. When defining methods or component level properties / state, then the names should be specific enough in the context of the entire component.
clearInterval(this.timerId); | ||
} | ||
|
||
pauseNotification() { |
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.
dismissTaskLagNotification
constructor(props) { | ||
super(props); | ||
this.state = { | ||
canShowLagError: 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.
canShowTaskLagNotification
` | ||
}); | ||
const PAUSE_INTERVAL_IN_MS = 1000 * 60 * 60; | ||
const LAG_THRESHOLD_IN_MS = 1000 * 60 * 3; |
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.
DISMISS_TASK_LAG_NOFICATION_DURATION_IN_MS
MAX_TASK_LAG_NOTIFICATION_THRESHOLD_IN_MS
|
||
pauseNotification() { | ||
this.setState({ canShowLagError: false }); | ||
this.timerId = setInterval(() => { |
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 should be setTimeout since it only needs to happen once
|
||
pauseNotification() { | ||
this.setState({ canShowLagError: false }); | ||
this.timerId = setInterval(() => { |
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.
reenableTaskLagNotificationTimeoutId
🚢 |
When a user dismisses the lag warning/error, it won't reappear for another hour and only if it's still lagging, or until they refresh the app.