Add a UI4 modal #4642
Add a UI4 modal #4642
Conversation
swansontec
force-pushed
the
william/ui4-modal
branch
from
1baf631
to
8800d18
Compare
src/components/ui4/ModalUi4.tsx
Outdated
| bridge: AirshipBridge<T> | ||
| children?: React.ReactNode | ||
|
|
||
| // Called when the user taps outside the modal or clicks the back button: |
There was a problem hiding this comment.
| // Called when the user taps outside the modal or clicks the back button: | |
| // Called when the user taps outside the modal, clicks the back button, or drags the modal down: |
| borderTopLeftRadius: theme.rem(1), | ||
| borderTopRightRadius: theme.rem(1), | ||
| flexShrink: 1, | ||
| width: theme.rem(30) // This works as a maxWidth because flexShrink is set |
There was a problem hiding this comment.
Is this good for tablets?
There was a problem hiding this comment.
Edit: Comment is out of scope for this review. We have an upcoming discussion next week regarding tablet layouts
|
Would like to see in this PR a blanket replacement of the original modals, shouldn't be any issues in doing this? |
swansontec
force-pushed
the
william/ui4-modal
branch
from
8800d18
to
63c487b
Compare
src/components/ui4/ModalUi4.tsx
Outdated
| const theme = useTheme() | ||
| const styles = getStyles(theme) | ||
|
|
||
| const padding = sidesToPadding(mapSides(fixSides(paddingRem, 1), theme.rem)) |
There was a problem hiding this comment.
In UI3, we had scenes with (mostly) 0rem padding, and modals (I think mostly if not all) had 1rem padding. This made component reuse tricky in UI3 between scenes and modals.
UI4 components all expect 0.5rem margins at a minimum on their modal/scene parents and neighbors/siblings. Specifically margins instead of padding, because using margins allows for children to set negative margins for the few exceptions where we need full widths:
- Full width dividers (trying to stop using these in the future according to Madison, whitespace is key to cleanliness)
- Carousels
Let me know if there's something super wrong about the above.
| warning?: boolean | ||
|
|
||
| // Called when the user taps outside the modal or clicks the back button. | ||
| // If this is missing, the modal will not be closable. |
There was a problem hiding this comment.
I'm assuming this is for modals that can only be dismissed after user checks an
"I understand" checkbox or similar. I found this pattern inconvenient in UI3.
Since this is the exception, not the rule, suggest we make the default behavior
taken care of by the modal (gracefully closing) without extra empty onCancel code from the
caller, and have a special optional prop lockUntilAction, persistent, nonExitable, completionRequired, exitRestricted, etc that blocks this default behavior.
Only need to update the few callers that have this special case, and leave the
rest as-is even though onCancel is no longer needed.
There was a problem hiding this comment.
There is no way to make the modal self-dismiss, since the modal doesn't have access to the Airship promise. You must handle the onCancel callback by doing a bridge.resolve, or the promise will be left dangling forever. This change simply aligns the visual with this existing reality.
| borderTopLeftRadius: theme.rem(1), | ||
| borderTopRightRadius: theme.rem(1), | ||
| flexShrink: 1, | ||
| width: theme.rem(30) // This works as a maxWidth because flexShrink is set |
There was a problem hiding this comment.
Edit: Comment is out of scope for this review. We have an upcoming discussion next week regarding tablet layouts
src/theme/variables/edgeDark.ts
Outdated
| @@ -550,6 +550,8 @@ export const edgeDark: Theme = { | |||
| start: { x: 1, y: 0 } | |||
| }, | |||
|
|
|||
| modalBackgroundUi4: '#00000060', | |||
There was a problem hiding this comment.
No need for palette colors? I'm fine either way, would love to avoid the extra boilerplate if something isn't reused.
| top: 0 | ||
| }, | ||
| dragBar: { | ||
| // TODO: Replace this color we have a proper design: |
There was a problem hiding this comment.
Shot a mention to Madison, we might as well get it all in before merge to avoid another change
Jon-edge
force-pushed
the
william/ui4-modal
branch
2 times, most recently
from
40772b7
to
258cc65
Compare
paullinator
force-pushed
the
william/ui4-modal
branch
from
258cc65
to
0d1af15
Compare
paullinator
force-pushed
the
william/ui4-modal
branch
from
0d1af15
to
413789e
Compare
paullinator
force-pushed
the
william/ui4-modal
branch
from
413789e
to
91f1a7a
Compare
New changes (with one updated modal per new built-in "padding"):
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneRequirements
If you have made any visual changes to the GUI. Make sure you have: