fix(app): allow navbar during LegacyModals on the modal portal #15166
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
sfoster1
added a commit
that referenced
this pull request
May 13, 2024
The implementation of the intervention modal (i.e., manual move labware) in the desktop app was sort of manually open-coded in the intervention modal organism. We're going to need modals styled in this way elsewhere, so split it out into an app molecule with some overridable styles. Also, make its background wrapper position: absolute instead of position: fixed, for the same reasons as #15166 - when this modal is hung off of the modal portal rather than the top portal, it should allow interaction with the navbar and the breadcrumbs. This should not affect the modal's use in the actual InterventionRequired organism, since in the organism the modal is hung off of the top portal. Closes RSQ-6
sfoster1
added a commit
that referenced
this pull request
May 13, 2024
The implementation of the intervention modal (i.e., manual move labware) in the desktop app was sort of manually open-coded in the intervention modal organism. We're going to need modals styled in this way elsewhere, so split it out into an app molecule with some overridable styles. Also, make its background wrapper position: absolute instead of position: fixed, for the same reasons as #15166 - when this modal is hung off of the modal portal rather than the top portal, it should allow interaction with the navbar and the breadcrumbs. This should not affect the modal's use in the actual InterventionRequired organism, since in the organism the modal is hung off of the top portal. Closes RSQ-6
3 tasks
sfoster1
added a commit
that referenced
this pull request
May 13, 2024
The implementation of the intervention modal (i.e., manual move labware) in the desktop app was sort of manually open-coded in the intervention modal organism. We're going to need modals styled in this way elsewhere, so split it out into an app molecule with some overridable styles. Also, make its background wrapper position: absolute instead of position: fixed, for the same reasons as #15166 - when this modal is hung off of the modal portal rather than the top portal, it should allow interaction with the navbar and the breadcrumbs. This should not affect the modal's use in the actual InterventionRequired organism, since in the organism the modal is hung off of the top portal. Closes RSQ-6
mjhuff
approved these changes
May 13, 2024
This is the only legacy modal on the modal portal that actually wants to continue having a non-interactive navbar
sfoster1
added a commit
that referenced
this pull request
May 14, 2024
The implementation of the intervention modal (i.e., manual move labware) in the desktop app was sort of manually open-coded in the intervention modal organism. We're going to need modals styled in this way elsewhere, so split it out into an app molecule with some overridable styles. Also, make its background wrapper position: absolute instead of position: fixed, for the same reasons as #15166 - when this modal is hung off of the modal portal rather than the top portal, it should allow interaction with the navbar and the breadcrumbs. This should not affect the modal's use in the actual InterventionRequired organism, since in the organism the modal is hung off of the top portal. Closes RSQ-6
1 task
sfoster1
added a commit
that referenced
this pull request
May 17, 2024
The implementation of the intervention modal (i.e., manual move labware) in the desktop app was sort of manually open-coded in the intervention modal organism. We're going to need modals styled in this way elsewhere, so split it out into an app molecule with some overridable styles. Also, make its background wrapper position: absolute instead of position: fixed, for the same reasons as #15166 - when this modal is hung off of the modal portal rather than the top portal, it should allow interaction with the navbar and the breadcrumbs. This should not affect the modal's use in the actual InterventionRequired organism, since in the organism the modal is hung off of the top portal. Closes RSQ-6 ## Testing - [x] The new storybook component should render - [x] Move labware and pause modals on desktop should still render as in ~figma~ the shipping app - [x] Move labware and pause modals on desktop should continue to ~(a) visually overlap and (b)~ inhibit interaction with the navbar and breadcrumbs the shipping app has these modals confined to the route component area, while figma has them overlapping everything with a small padding to the viewport boundary. this pr doesn't change that. the navbar and route breadcrumbs are still non interactive.
Carlos-fernandez
pushed a commit
that referenced
this pull request
May 20, 2024
The implementation of the intervention modal (i.e., manual move labware) in the desktop app was sort of manually open-coded in the intervention modal organism. We're going to need modals styled in this way elsewhere, so split it out into an app molecule with some overridable styles. Also, make its background wrapper position: absolute instead of position: fixed, for the same reasons as #15166 - when this modal is hung off of the modal portal rather than the top portal, it should allow interaction with the navbar and the breadcrumbs. This should not affect the modal's use in the actual InterventionRequired organism, since in the organism the modal is hung off of the top portal. Closes RSQ-6 ## Testing - [x] The new storybook component should render - [x] Move labware and pause modals on desktop should still render as in ~figma~ the shipping app - [x] Move labware and pause modals on desktop should continue to ~(a) visually overlap and (b)~ inhibit interaction with the navbar and breadcrumbs the shipping app has these modals confined to the route component area, while figma has them overlapping everything with a small padding to the viewport boundary. this pr doesn't change that. the navbar and route breadcrumbs are still non interactive.
Carlos-fernandez
pushed a commit
that referenced
this pull request
May 20, 2024
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
Carlos-fernandez
pushed a commit
that referenced
this pull request
Jun 3, 2024
The implementation of the intervention modal (i.e., manual move labware) in the desktop app was sort of manually open-coded in the intervention modal organism. We're going to need modals styled in this way elsewhere, so split it out into an app molecule with some overridable styles. Also, make its background wrapper position: absolute instead of position: fixed, for the same reasons as #15166 - when this modal is hung off of the modal portal rather than the top portal, it should allow interaction with the navbar and the breadcrumbs. This should not affect the modal's use in the actual InterventionRequired organism, since in the organism the modal is hung off of the top portal. Closes RSQ-6 ## Testing - [x] The new storybook component should render - [x] Move labware and pause modals on desktop should still render as in ~figma~ the shipping app - [x] Move labware and pause modals on desktop should continue to ~(a) visually overlap and (b)~ inhibit interaction with the navbar and breadcrumbs the shipping app has these modals confined to the route component area, while figma has them overlapping everything with a small padding to the viewport boundary. this pr doesn't change that. the navbar and route breadcrumbs are still non interactive.
Carlos-fernandez
pushed a commit
that referenced
this pull request
Jun 3, 2024
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.
- 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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, allowsleft:,right:,top:,bottom:, etc) but has its positioning parent be its DOM parent, which in this case is the modal portal element, which means thatLegacyModalis instantiated viacreatePortalto the "top level portal root", it will occlude and intercept interaction to everything in the viewport (also previous behavior, correct)LegacyModalis instantiated viacreatePortalto the "modal portal root", it will occlude and intercept interaction to the page content but not the navbar or breadcrumbs (new behavior, correct)LegacyModalis 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
LegacyModal: