fix(app): put blocking user flows in new top level modal portal #6821
Conversation
There are a number of new calibration calibration flows that launch a wizard style modal in response to creating a calibration session on the robot. These modals were, like all existing modals in the app, rendered into a portal component that is a sibling of the two side panel components in the main layout component. This meant that users were able to interact with the navigation tabs and the contents of the sidepanel during these captive flows that involve precise coordination and movement on the robot. In order protect against uncharacterized behavior when users navigate away from the the current tab during a calibration user flow, this adds a new top level modal portal. Modals rendered in this portal will span the entire window of the app, and provide assurance that a session will be deleted any time a user navigates away from, or completes a calibration user flow.
b-cooper
added
app
b-cooper
requested review from
a team,
mcous and
Laura-Danielle
and removed request for
a team
| @@ -91,6 +91,7 @@ export function App(): React.Node { | |||
| <ModalPortalRoot /> | |||
| <Alerts /> | |||
| </Box> | |||
| <TopPortalRoot /> | |||
There was a problem hiding this comment.
This looks like it will render over <Alerts />, which is persistent app alerts that will be rendered regardless of URL route (currently, app update available alert and USB to Ethernet driver outdated alert).
The intention of <Alerts> is to have global app alerts, so I don't think this should render over them
There was a problem hiding this comment.
That's a good point. If the intention of the <Alerts> is to be global as well, then perhaps the solution is also move them outside of the page Box?
There was a problem hiding this comment.
Yeah, something like that could make a lot of sense / might be necessary if this is a larger UI direction we're taking with the app
There was a problem hiding this comment.
Tested the app flows; the only kinda weird one is change pipette where you go from a page portal that drives the change pipette flow to a top portal for calibration, which also has that problem we know about where the portal goes away while the session is starting, but I think it's still better.
…ndow-modal-portal
Codecov Report
@@ Coverage Diff @@
## chore_release-4.0.0-beta.0 #6821 +/- ##
=============================================================
Coverage ? 89.05%
=============================================================
Files ? 101
Lines ? 4521
Branches ? 0
=============================================================
Hits ? 4026
Misses ? 495
Partials ? 0 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
After thinking through this some more, I don't think this is the best approach we have available to us. Approving for user-testing beta build purposes.
A state-connected component that can determine if it should render based on API session state from Redux, with enough props passed in from its parent to account for the different UI contexts in which it's placed, should solve the problem as it's laid out in the PR description (I think) in a more robust manner.
Overview
There are a number of new calibration calibration flows that launch a wizard style modal in response
to creating a calibration session on the robot. These modals were, like all existing modals in the
app, rendered into a portal component that is a sibling of the two side panels in the main
Applayout component.This meant that users were able to interact with the navigation tabs and the
contents of the side panel during these captive flows that involve precise coordination and movement
on the robot.
In order protect against uncharacterized behavior when users navigate away from the
the current tab during a calibration user flow, this adds a new top level modal portal. Modals
rendered in this portal will span the entire window of the app, and provide assurance that a session
will be deleted any time a user navigates away from, or completes a calibration user flow.
Changelog
levelprop to the app'sPortalcomponent. It will default to the string "page" which specifies the existing portal level.SidePanelandNavBarcomponents. (one level higher than the page)level="top"prop to all modals within calibration overhaul flows.Review requests
Risk assessment
Medium, although this doesn't effect modals that exist outside of the new calibration overhaul code, this type of block modal UX hasn't existed in the app before. By introducing a new paradigm we risk trapping users on the modal if something were to go wrong in deleting the session. (users might have previously just navigated away and come back to a tab)