feat: Orbit banner warning for stale data#523
Conversation
…test code in the actual context provider
…ion details in the context
… fix external scrolling issue
…te consistent width
… re-render shouldn't be based on all the variables within the effect
…is expected on non-ladder pages
andrewdolce
left a comment
andrewdolce
left a comment
There was a problem hiding this comment.
Couple of questions, but overall looks solid!
| "flex overflow-auto transition-all duration-300 ease-in-out w-full", | ||
| sideBarSelection && "ml-80 min-[1485px]:ml-0", | ||
| "flex transition-all duration-300 ease-in-out overflow-x-auto w-full", | ||
| sideBarSelection && "min-[1485px]:ml-30", |
There was a problem hiding this comment.
issue/suggestion: I realized in testing around that ml-30 actually isn't a value supported by tailwind by default, so this actually gets ignored. As spacing gets larger, at a certain point, it only pre-defines classes in certain increments. So either we should pick ml-32 here, or use a custom value in rem or px.
There was a problem hiding this comment.
Good callout. This conditional styling is actually no longer necessary since the sidebar is no longer out of the document flow; flex box should handle the re-sizing/spacing. Removed.
| ); | ||
| } | ||
| }, [topic, event, channel, parser, RawData]); | ||
| }, [topic, event, parser, channel, RawData]); |
There was a problem hiding this comment.
nit: Is there a reason this changed? It's fine either way but if this wasn't intentional maybe drop it from the PR so it doesn't pollute git blame.
There was a problem hiding this comment.
Good catch, the order swapping was unintentional. Updated
| const addWarning: (warning: DataWarning) => void = (warning: DataWarning) => { | ||
| setWarnings((warnings) => { | ||
| const newSet = new Set(warnings); | ||
| newSet.add(warning); | ||
| return newSet; | ||
| }); | ||
| }; | ||
| const removeWarning: (warning: DataWarning) => void = ( | ||
| warning: DataWarning, | ||
| ) => { | ||
| setWarnings((warnings) => { | ||
| const newSet = new Set(warnings); | ||
| newSet.delete(warning); | ||
| return newSet; | ||
| }); | ||
| }; |
There was a problem hiding this comment.
praise: I like that you broke this out rather than just copying the basic setter that Glides has. Seems nice that each caller doesn't need to do its own Set diff.
| // Disabling this because it's due to outdated react-hooks lint rules | ||
| // Remove when that library is updated | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
question: Interesting. So this is something that is different in later versions of eslint? I see that it otherwise complains that checkForStaleData should be declared as a dependency of this useEffect. Maybe related to the use of useEffectEvent, but can you provide any context on why it shouldn't be a dependency?
There was a problem hiding this comment.
This has to do with useEffectEvents not being "stable" (i.e. they change on every render) so including it in the dependency array would cause it to re-fire on every render. Apparently this is an intended way of preventing an incorrect use of useEffectEvent. Newer version of ESLint will actually warn if you include it in the dependency array.
There's an explanation of the reasoning behind this here: https://react.dev/reference/react/useEffectEvent#why-are-effect-events-not-stable.
There was a problem hiding this comment.
After reading through this, I think I understand and agree with the use of the effect here. From perusing eslint-plugin-react-hooks I think some of their later releases (possibly 7.1.0) address this? Unclear. In any case I'm happy with this for now until we update.
| RawData: VehicleDataMessage, | ||
| defaultResult: null, | ||
| }); | ||
| const checkForStaleData = useEffectEvent(() => { |
There was a problem hiding this comment.
question: I haven't personally delved into useEffectEvent, and I don't think we have examples of it being used elsewhere. Which doesn't mean that you can't/shouldn't use it here, but could you maybe explain the benefits as opposed to just putting this code directly inside the useEffect call?
There was a problem hiding this comment.
useEffectEvent is new in React 19 so this is also my first time using it.
As I understand it from reading the doc (https://react.dev/reference/react/useEffectEvent), useEffectEvent is for handling firing events from Effects where the functions related to that firing aren't related to the re-rendering logic of the useEffect. In this case addWarnings and setWarnings aren't related to when this useEffect should be re-triggered but would have to be wrapped in a useCallback and included in the dependency array anyway if they weren't separated out into a useEffectEvent.
Although in typing the above and thinking through this again, mostRecentTimestamp should be included in the dependency array so that we don't run into a situation where the stale data banner lingers after data starts flowing again because we're only checking every 1 minute. Will cause more renders which I was trying to avoid but it's probably the correct logic and not including it is kind of an anti-pattern that the useEffectEvent docs advises against. Will update that.
…avoid muddling git blame history
…r expanded in ladder page
| // Disabling this because it's due to outdated react-hooks lint rules | ||
| // Remove when that library is updated | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
After reading through this, I think I understand and agree with the use of the effect here. From perusing eslint-plugin-react-hooks I think some of their later releases (possibly 7.1.0) address this? Unclear. In any case I'm happy with this for now until we update.
2 participants
Asana Task: 🪐 📐 Orbit banner warning for stale data
Add warning banner to top of page if data is stale (>3 minutes since there's been a position update).
Includes significant changes to header/sidebar alignment; changed from
fixedand updatedflexproperties to accommodate that removal. Thefixedheader and sidebar created issues when content was dynamically added above it.Checklist
(N/A)Light & dark mode(X)Desktop & mobile sizes(X)Chromium( )Firefox( )Safari(X)Commits free of internal data(X)PR description free of internal data(X)Logging free of internal data(X)Has tests( )Doesn't need tests( )Tests deferred (with justification)( )Okayed the plan for the feature (e.g. the design files, or the Asana task)( )Reviewed the feature as implemented (e.g. on dev-green, or saw screenshots)( )No review needed