Skip to content
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

Merged
merged 41 commits into from
Jun 19, 2024
Merged

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented May 24, 2024

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:

@Janpot Janpot added the core Infrastructure work going on behind the scenes label May 24, 2024
@Janpot Janpot changed the title Implement notofications/dialogs RFCs Implement notifications/dialogs RFCs May 24, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 28, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 5, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 12, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 12, 2024
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 12, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 13, 2024
@Janpot Janpot marked this pull request as ready for review June 13, 2024 13:54
@Janpot Janpot requested a review from a team June 13, 2024 13:54
Copy link
Member

@apedroferreira apedroferreira left a 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.

import { DialogsProvider } from '@toolpad/core/useDialogs';

function App({ children }) {
return <DialogsProvider>{children}</DialogsProvider>;
Copy link
Member

@apedroferreira apedroferreira Jun 14, 2024

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?

Copy link
Member Author

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}>
Copy link
Member

@apedroferreira apedroferreira Jun 14, 2024

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)

Copy link
Member Author

@Janpot Janpot Jun 16, 2024

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
Copy link
Member

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.

Copy link
Member Author

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.

packages/toolpad-core/src/useDialogs/DialogsProvider.tsx Outdated Show resolved Hide resolved
Janpot and others added 3 commits June 14, 2024 19:01
@Janpot Janpot requested a review from apedroferreira June 15, 2024 11:55
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 16, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 18, 2024
@Janpot Janpot merged commit a8fa780 into mui:master Jun 19, 2024
13 checks passed
@Janpot Janpot deleted the notifications-dialogs branch June 19, 2024 08:07
@oliviertassinari
Copy link
Member

If we ever need to go one level down in the abstraction: https://github.com/desko27/react-call is cool.

@oliviertassinari
Copy link
Member

https://github.com/sweetalert2/sweetalert2 shows the potential of getting this right. 17K stars, 200K sessions/month, 1600 closed issues. Insane

@oliviertassinari oliviertassinari added component: dialog This is the name of the generic UI component, not the React module! component: snackbar This is the name of the generic UI component, not the React module! labels Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: dialog This is the name of the generic UI component, not the React module! component: snackbar This is the name of the generic UI component, not the React module! core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants