-
-
Notifications
You must be signed in to change notification settings - Fork 310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement notifications/dialogs RFCs #3584
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this API looks really good and complete! I've compared it with some modal and toast components/hooks I've done for dashboards before and looks like it covers all the same cases, even async actions with loading state and disabling buttons/closing.
Just found some typos and left some thoughts on a couple of things.
docs/data/toolpad/core/components/use-dialogs/CustomDialogWithPayload.js
Outdated
Show resolved
Hide resolved
docs/data/toolpad/core/components/use-dialogs/CustomDialogWithResult.js
Outdated
Show resolved
Hide resolved
import { DialogsProvider } from '@toolpad/core/useDialogs'; | ||
|
||
function App({ children }) { | ||
return <DialogsProvider>{children}</DialogsProvider>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How common is it going to be that someone has to import the provider directly instead of just using AppProvider
?
That's why so far I haven't documented the branding and navigation providers like this, but maybe it could be done for consistency, or maybe dialogs and notifications might have more specific use cases than those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The notifications and dialogs can stand perfectly on their own. The branding and navigation providers are just implementation details of the app provider. Internal until tehre is a good reason not to.
export default function ScopedNotification() { | ||
return ( | ||
<Box sx={{ width: '100%', height: 150, position: 'relative' }}> | ||
<NotificationsProvider slots={notificationsProviderSlots}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another possibility could be to somehow pass an optional target to render on when calling notifications.show
instead of using nested providers (which I guess kind of nullifies the top-level NotificationsProvider
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how that would work, the provider takes care of queueing and deduplicating multiple concurrent notifications. Perhaps over time we could introduce a top-level API similar to notistach or sonner
import { Notifications, notifications } from '@mui/toolpad/notifications'
// put <Notifications /> in your app layout
// use notifications.show(...) across the codebase
// and/or
import { setup } from '@mui/toolpad/notifications'
const { Notifications, notifications } = setup()
// put <Notifications /> in your app
// use notifications.show(...) across the codebase
// useful for scoping notifications to a specific part of the app as well as having global ones
|
||
{{"demo": "AlertNotification.js"}} | ||
|
||
## Multiple notifications |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could stack them vertically, but that could take some work to do well so could just be a later improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a section that talks about "notification center". a way to view and manage all open notifications at once.
docs/data/toolpad/core/components/use-notifications/use-notifications.md
Outdated
Show resolved
Hide resolved
docs/data/toolpad/core/components/use-dialogs/use-dialogs-api.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Pedro Ferreira <[email protected]> Signed-off-by: Jan Potoms <[email protected]>
…Payload.js Co-authored-by: Pedro Ferreira <[email protected]> Signed-off-by: Jan Potoms <[email protected]>
If we ever need to go one level down in the abstraction: https://github.com/desko27/react-call is cool. |
https://github.com/sweetalert2/sweetalert2 shows the potential of getting this right. 17K stars, 200K sessions/month, 1600 closed issues. Insane |
Implements RFCs:
I decided to handwrite the hook API docs to make them as comprehensive as possible. We can search for ways to generate them but I doubt it will be easy to make something that matches the hand-written version.
Docs: