Skip to content

Commit

Permalink
fix(app): allow navbar during LegacyModals on the modal portal (#15166)
Browse files Browse the repository at this point in the history
In theory, the intent both from a programming and a design aspect of
having the "modal portal root" - the portal component that lives inside
the desktop app's route-component render node - was that modals
registered to the modal portal instead of the top portal would occlude
page content (i.e. the Devices page, a protocols page, whichever one
that modal said it wanted) but _not_ occlude or inhibit interaction with
the navbar or the breadcrumbs.

However, because the Overlay component behind LegacyModal (which is the
thing that renders the translucent occlusion of background content, and
the thing that intercepts interaction) had `position: fixed`, it would
always be positioned as-if its parent were the viewport, meaning that it
would always occlude interaction with the entire app, even while
centering the actual modal content on the page content. This is wrong.

The solution is for Overlay to be `position: absolute`, which has most
of the same semantics (also creates a z-order stack, allows `left:`,
`right:`, `top:`, `bottom:`, etc) but has its positioning parent be its
DOM parent, which in this case is the modal portal element, which means
that
- When a `LegacyModal` is instantiated via `createPortal` to the "top
level portal root", it will occlude and intercept interaction to
everything in the viewport (also previous behavior, correct)
- When a `LegacyModal` is instantiated via `createPortal` to the "modal
portal root", it will occlude and intercept interaction to the page
content but not the navbar or breadcrumbs (new behavior, correct)
- When a `LegacyModal` is instantiated via a normal component render
tree, it will occlude and intercept interaction to whatever is below it
in the component tree (new behavior, correct - previously this would
also intercept and occlude the entire viewport)

This is a lot of words for a small css change, but it affects a whole
lot of things.

## Review and Testing Requests
- There should be no change to modals that use the top root, let's smoke
test this and check; that should include app update notifications and
estop modals
- We should smoke test the following modals, which use the modal portal
and a `LegacyModal`:
    - [x] Confirm cancelling a run
    - [x] Robot update notifications
  • Loading branch information
sfoster1 authored and Carlos-fernandez committed May 20, 2024
1 parent b2fd523 commit b3c120d
Show file tree
Hide file tree
Showing 3 changed files with 5 additions and 6 deletions.
3 changes: 1 addition & 2 deletions app/src/molecules/LegacyModal/LegacyModalShell.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import * as React from 'react'
import styled from 'styled-components'
import {
COLORS,
POSITION_FIXED,
POSITION_ABSOLUTE,
ALIGN_CENTER,
JUSTIFY_CENTER,
Expand Down Expand Up @@ -76,7 +75,7 @@ export function LegacyModalShell(props: LegacyModalShellProps): JSX.Element {
}

const Overlay = styled.div`
position: ${POSITION_FIXED};
position: ${POSITION_ABSOLUTE};
left: 0;
right: 0;
top: 0;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
getRobotUpdateAvailable,
} from '../../../../redux/robot-update'
import { getAvailableShellUpdate } from '../../../../redux/shell'
import { getModalPortalEl } from '../../../../App/portal'
import { getTopPortalEl } from '../../../../App/portal'
import { UpdateAppModal } from '../../../../organisms/UpdateAppModal'
import { MigrationWarningModal } from './MigrationWarningModal'
import { UpdateRobotModal } from './UpdateRobotModal'
Expand Down Expand Up @@ -60,7 +60,7 @@ export function ViewUpdateModal(
if (availableAppUpdateVersion && showAppUpdateModal)
return createPortal(
<UpdateAppModal closeModal={() => setShowAppUpdateModal(false)} />,
getModalPortalEl()
getTopPortalEl()
)

if (showMigrationWarning) {
Expand Down
4 changes: 2 additions & 2 deletions app/src/organisms/RunDetails/ConfirmCancelModal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
} from '@opentrons/api-client'
import { useStopRunMutation } from '@opentrons/react-api-client'

import { getModalPortalEl } from '../../App/portal'
import { getTopPortalEl } from '../../App/portal'
import { LegacyModal } from '../../molecules/LegacyModal'
import { useTrackProtocolRunEvent, useIsFlex } from '../Devices/hooks'
import { useRunStatus } from '../RunTimeControl/hooks'
Expand Down Expand Up @@ -108,6 +108,6 @@ export function ConfirmCancelModal(
</Flex>
</Flex>
</LegacyModal>,
getModalPortalEl()
getTopPortalEl()
)
}

0 comments on commit b3c120d

Please sign in to comment.