Adding modal into blueprint with tests #34
Conversation
|
|
||
| export default function Modal({ children, onCloseCallback, closeDisabled, actionChildren, ...rest }: ResponsiveModalProps) { | ||
| const theme = useTheme(); | ||
| const fullScreen = useMediaQuery(theme.breakpoints.down('md')); |
There was a problem hiding this comment.
an optional fullScreen prop on Modal would be nice to allow overriding this behavior. as i can see some occasions where we'd want full screen on large screens, or not want full screen on smaller screens.
|
|
||
| return ( | ||
| <Dialog {...rest} fullScreen={isFullScreen} onClose={onClose}> | ||
| <DialogContent>{children}</DialogContent> |
There was a problem hiding this comment.
This doesn't really have any provision for a Title, like a DialogTitle
There was a problem hiding this comment.
Unless the intent is for it to exist in DialogContent. I would think even if that was the case we'd want both like you have for actionChildren &&
There was a problem hiding this comment.
Yea, the intent was to just throw something in DialogContent. I figured the title would just be a string so thats easy to add and style how you want. But the actionChildren has specific styling and padding with it
There was a problem hiding this comment.
To be honest I didn't really even want to include the action children, but felt it was necessary just in case
There was a problem hiding this comment.
I'd prob lean toward's Austin side, using MUI we should lean into the existing components/APIs it has when we can, and just to have full 1-to-1 parity with what you get out of the box. 99% of the time itll be a simple string yeah, but i can also think of cases where we've had html entities/custom UI in a modal title.
Usually always better to make "slot" props ReactNode's, as it allows string, but also everything else too.
There was a problem hiding this comment.
Yeah that works.
Though I will call out that my decision to include Title in Content in PLUT has had some negative ramifications with things like padding and margin on the content impacting (negatively) the layout with the title
app/src/core/components/Modal.tsx
Outdated
|
|
||
| export default function Modal({ title, children, onCloseCallback, closeDisabled, actions, fullScreen, ...rest }: ResponsiveModalProps) { | ||
| const theme = useTheme(); | ||
| const isFullScreen = fullScreen ?? useMediaQuery(theme.breakpoints.down('md')); |
There was a problem hiding this comment.
not sure if you saw my slack thread comments on this, but i dont think you actually need to define fullScreen here or in your props interface, since Dialog already has it. Also this might technically be invalid in strict react rendering since hooks are not supposed to be conditionally invoked.
You should be able to remove all those changes and just do
<Dialog fullScreen={fullScreen} {...rest}
No description provided.