Dynamic paging intervals for elevator screens #2402
Conversation
| @@ -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.
This was something I originally talked about with Cora, but there isn't really an equivalent for lastUpdate to trigger the hook in wrappedComponent. Not sure if this wrapper makes as much sense anymore to use for the elevator screen components
| useEffect(() => { | ||
| if (pageIndex === 0) onFinish(); | ||
| }, [pageIndex]); |
There was a problem hiding this comment.
This needs to be a separate hook relying on pageIndex because you cannot modify state within a component (ComponentWrapper) from a callback function in a different component. However, you can do this from a hook. So even though this seems like it would make more sense to be done in advancePage, it needs to be in its own hook.
There was a problem hiding this comment.
In the abstract I think it does make sense even to use for paging components that don't trigger paging on data refresh — to avoid confusion or the appearance of a "glitchy" display we want them to get through all their pages before updated data potentially changes the arrangement of items or number of pages. For the concrete case of elevator screens, this data doesn't change much moment-to-moment, so maybe it's not that essential. Wrapped components are free to ignore / not match on this prop, of course, so in that sense it is already optional.
| @@ -3,7 +3,7 @@ import { LastFetchContext } from "Components/v2/screen_container"; | |||
|
|
|||
| interface WrappedComponentProps { | |||
| onFinish: () => void; | |||
| lastUpdate: number | null; | |||
| lastUpdate?: number | null; | |||
There was a problem hiding this comment.
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 do lastUpdate?: number if the changes here make that the easier way.
There was a problem hiding this comment.
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
|
|
||
| // Cleanup the timer interval when the component unmounts or dependencies change | ||
| useEffect(() => { | ||
| if (!advanceOnDataRefresh && cycleIntervalMs) { |
There was a problem hiding this comment.
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 useClientPaging is ~25 lines of code), might it be worth splitting into two separate hooks intead? Something like useRefreshPaging and useIntervalPaging. (I realize this is somewhat of a different approach from what I suggested at a high level previously, which was to combine these behaviors into a single hook with a flag for which one to use — sometimes it's not obvious how things will shake out until you build them 😅)
There was a problem hiding this comment.
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!
Asana task: Set up dynamic paging intervals for elevator screens
Description