refactor(app): Refactor intervention modal render behavior #15898
Conversation
| /* Props that must be truthy for the modal to render. Prevents multiple null checks while guaranteeing prop type safety. */ | ||
| modalProps: Omit<InterventionModalProps, 'onResume'> | null | ||
| } { | ||
| return React.useMemo(() => { |
There was a problem hiding this comment.
why do we need to cache this?
There was a problem hiding this comment.
Thanks for reviewing!
Yeah, good Q. We probably don't to be honest. I can't imagine there's any tangible, positive performance impact to be gained here.
mjhuff
force-pushed
the
app_refactor-intervention-modal-render-logic
branch
from
19c1e4b to
3bc6399
Compare
| alignItems: ALIGN_CENTER, | ||
| justifyContent: JUSTIFY_SPACE_BETWEEN, | ||
| } as const | ||
| // If showModal is true, modalProps are guaranteed not to be null. |
There was a problem hiding this comment.
can tell typescript this by having it return { showModal: false, modalProps: null } | {showModal: true, modalProps: Omit<InterventionModalProps, 'onResume'>} right?
There was a problem hiding this comment.
or if you mean that the elements won't be null, you'll have to go a little more in depth - it might be worth having those props be | undefined so you can use Partial and Required
There was a problem hiding this comment.
Ahhhhh, really good catch - I totally blanked on this. The former type is exactly what we need here.
(Hopefully) closes RQA-2904
Overview
The logic for rendering
InterventionModalon the ODD/Desktop is a little bit different when looking at the exact conditions, and this (likely) causes theInterventionModalto render on the ODD sometimes but not on the desktop app, and vice versa.This is a good opportunity to refactor all of this logic into its own hook and use that hook where we render
InterventionModal. After thinking through the render logic, there's room to simplify it a bit, too. We don't actually need stateful storage of an intervention command key. Also, I decided to separateshowModalfrommodalProps(which lets us pass all the non-null props simply), even though we could technically just do a truthy check formodalPropsfor renderingInterventionModal, since this is maybe a bit more intuitive. Lastly, a few missing tests are added.To help with bug testing intervention modals, I added a couple
console.warns.Test Plan and Hands on Testing
Changelog
Risk assessment
low