-
Notifications
You must be signed in to change notification settings - Fork 2
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
Dynamic paging intervals for elevator screens #2402
base: main
Are you sure you want to change the base?
Changes from 9 commits
8b12921
769534c
d4357bc
9af497f
7845852
0d5c878
34db43e
ecfdccb
cfd595a
bb3cee3
dddddde
0b25cad
c05c36c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ import { LastFetchContext } from "Components/v2/screen_container"; | |
|
||
interface WrappedComponentProps { | ||
onFinish: () => void; | ||
lastUpdate: number | null; | ||
lastUpdate?: number | null; | ||
} | ||
|
||
interface Props { | ||
|
@@ -37,7 +37,7 @@ const PersistentWrapper: ComponentType<Props> = ({ | |
{...visibleData} | ||
onFinish={handleFinished} | ||
key={renderKey} | ||
lastUpdate={lastFetch} | ||
lastUpdate={lastFetch} // make this optional - either determined by lastFetch Context or by other interval | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was something I originally talked about with Cora, but there isn't really an equivalent for |
||
/> | ||
); | ||
}; | ||
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
import { useState, useEffect, useRef, useCallback } from "react"; | ||
|
||
interface UsePageAdvancerProps { | ||
numPages: number; // Total number of pages to cycle through | ||
advanceOnDataRefresh: boolean; // Whether to advance when data is refreshed | ||
cycleIntervalMs?: number; // In milliseconds, the interval for cycling pages (only used if advanceOnDataRefresh is false) | ||
lastUpdate?: number | null; | ||
onFinish: () => void; // Callback when cycling completes | ||
} | ||
|
||
/** | ||
* This hook acts as a way for components with multiple pages to advance | ||
* through their pages in one of two ways: | ||
* 1. Interval-based advancement (advances page every __ ms) | ||
* 2. Refresh-based advancement (advances page with every data refresh) | ||
*/ | ||
function usePageAdvancer({ | ||
numPages, | ||
cycleIntervalMs, | ||
advanceOnDataRefresh = false, | ||
lastUpdate, | ||
onFinish, | ||
}: UsePageAdvancerProps) { | ||
const [pageIndex, setPageIndex] = useState(0); | ||
const intervalRef = useRef<NodeJS.Timeout | null>(null); | ||
|
||
// Use refs to keep stable references to numPages and onFinish | ||
const numPagesRef = useRef(numPages); | ||
const onFinishRef = useRef(onFinish); | ||
|
||
useEffect(() => { | ||
numPagesRef.current = numPages; | ||
}, [numPages]); | ||
|
||
useEffect(() => { | ||
onFinishRef.current = onFinish; | ||
}, [onFinish]); | ||
|
||
// Callback that handles changing the state of pageIndex | ||
const advancePage = useCallback(() => { | ||
setPageIndex((prevIndex) => { | ||
const nextIndex = (prevIndex + 1) % numPagesRef.current; | ||
if (nextIndex === 0) onFinishRef.current(); // Call onFinish when cycling completes | ||
return nextIndex; | ||
}); | ||
}, []); | ||
|
||
// Start the interval for time-based advancement | ||
const startTimer = useCallback(() => { | ||
if (intervalRef.current) clearInterval(intervalRef.current); // Clear any existing timer | ||
if (cycleIntervalMs) { | ||
intervalRef.current = setInterval(() => { | ||
advancePage(); | ||
}, cycleIntervalMs); | ||
} | ||
}, [cycleIntervalMs, advancePage]); | ||
|
||
// Cleanup the timer interval when the component unmounts or dependencies change | ||
useEffect(() => { | ||
if (!advanceOnDataRefresh && cycleIntervalMs) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This hook has a lot of conditionals on whether we're using data-refresh paging or interval paging, and a few internal concepts that only matter when we're in one of the two "branches" — considering the code for each case on its own is pretty concise (the old There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This hook did end up being more confusing than I was expecting to get working for both cases within the same hook. It would likely be quite a bit simpler split out into two hooks, which I think makes up for the ease of use of having one hook for all paging. I think going back to this approach makes sense! |
||
startTimer(); | ||
return () => { | ||
if (intervalRef.current) clearInterval(intervalRef.current); | ||
}; | ||
} | ||
// Hooks must return void or a cleanup function, | ||
// so we explicitly return undefined when no cleanup is necessary. | ||
return undefined; | ||
}, [advanceOnDataRefresh, cycleIntervalMs, startTimer]); | ||
|
||
// Handle refresh-based advancement | ||
useEffect(() => { | ||
if (advanceOnDataRefresh && lastUpdate !== null) { | ||
advancePage(); | ||
} | ||
}, [lastUpdate]); | ||
|
||
return pageIndex; | ||
} | ||
|
||
export default usePageAdvancer; |
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 already defined as
| null
— it's a little confusing to have multiple ways to not provide a value for a prop. Perhaps we could just dolastUpdate?: number
if the changes here make that the easier way.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.
agreed this should not have both undefined and null. I think null makes this easier, but I had the undefined/optional leftover from my TODO. Will clean up