adding NotificationContainer to be more inline with Mantine#539
adding NotificationContainer to be more inline with Mantine#539AnnMarieW merged 24 commits intosnehilvj:masterfrom
NotificationContainer to be more inline with Mantine#539Conversation
utilizes the api more closely exposes the api and store at `appNotifications` in the `dash_mantine_components` namespace
|
https://mantine.dev/x/notifications/#subscribe-to-notifications-state Figured out a workaround for this, its not their describe way |
|
Here is an example app to show the difference in the API: |
…ations available at `dash_mantine_components.appNotifications.store`
|
fixes #525 |
…xt when it shouldnt be
|
Hey Alex - this PR is ready for review. It works well and looks like it resolves the issue from the original component. Would like to get your thoughts on the API and how to handle the transition from the current method to this one in order to avoid breaking changes (and confusion) |
| return () => { | ||
| delete appNotifications['api'] | ||
| delete appNotifications['store'] | ||
| } |
There was a problem hiding this comment.
Is it important to remove these on unmount? Thinking about two scenarios:
- A clientside callback that's set to perhaps clear the notifications in several situations, one of which also removes the
NotificationContainerentirely, depending on the order of operations the clientside callback may be executed after theNotificationContainerhas already unmounted. Feels like we could leave theapiandstorethere even if accessing them isn't going to do anything visible. - Changing layouts/pages, if the
NotificationContaineris in both layout contents rather than in the page skeleton. Can we always guarantee that the new one will be created after the previous one's unmount hooks have executed? If for any reason these happened in the opposite order we'd end up deleting them after creating them the second time and they'd end up gone. This one seems unlikely, unless you do something really weird like put the two layout chunks into different parts of the page skeleton so they're both briefly on screen simultaneously... nevertheless I feel like we could just leave these items where they are and avoid this possibility.
There was a problem hiding this comment.
I guess we could leave them, just not sure what error handlers that Mantine provides on them.
It's not recommended to have multiple notification providers in the app, so both of these scenarios shouldnt really happen.
There was a problem hiding this comment.
Not recommended is different from won't happen 😉
The user's goal in these scenarios is not to have multiple notification providers at the same time, it's to move the notification provider from one place to another - perhaps on one page you want them at the top level, so extending vertically down from the top right corner on top of other content, but on a different page you want something else always visible in the top right corner so you put the notifications inside a smaller container.
There was a problem hiding this comment.
The notification itself can be placed in different places, or you could have a clientside update based upon the layout you are loading. :)
| /** Determines whether notifications container should be rendered inside `Portal`, `true` by default */ | ||
| withinPortal?: boolean; | ||
| /** Notifications to be passed to the API */ | ||
| sendNotifications?: NotificationsFormat; |
There was a problem hiding this comment.
Overall I like this pattern, but most of the time you're just going to use {'show': [...]} - I wonder if it would be better as three separate props, like showNotifications, hideNotifications, updateNotifications?
There was a problem hiding this comment.
Yeah, we could do that.
There was a problem hiding this comment.
So, when working with this, its kinda frustrating have to split this into multiple props. I'll push the changes and let you look at it.
There was a problem hiding this comment.
Frustrating when you have logic that needs to decide between show and update or some such? I can see that, though I still feel like this is an edge case so I'm not too bothered by that being less convenient. Do you agree or do you think it's a common need?
An alternative would be to make one list of notifications, but give each one an extra optional key like action ?= 'show', 'update', 'hide'. That means the object no longer quite matches the Mantine notification format, but one extra key isn't so bad. Then the code to handle them would be something like:
useEffect(() => {
if (notifications) {
notifications.forEach((notification) => {
const {action, ...realNotification} = notification;
notifications[action || 'show'](newRenderDashComponents(realNotification, ['message', 'icon', 'title']));
});
setProps({ notifications: [] }); // Avoid duplicate processing
}
}, [notifications]);There was a problem hiding this comment.
That is how the original dmc api was done.
I think it feels better this way, especially with the default of show.
There was a problem hiding this comment.
Actaully, I also think it would be better to have separate props for show update and hide (rather than sendNotificaitons) It's a closer match to the Mantine API. It also makes it easier to know what props each action requires. For example, hide only needs the notification id, show and update can take any of the valid Notification props. Currently if you use an invalid action you get a very hard to debug error message. Separate props would ensure only the valid Mantine function is used.
Bryan has made a good case that it's more convenient if it's all in one prop (sendNotifications), but a workaround for that could be using setProps or the api directly in a clientside callback using the new appNotifications
There was a problem hiding this comment.
This is the current way that I have set_props working for my app, notice that it just targets the children prop:
def app_notification(notification_type, message, ctx=None, nid=None):
"""
Adds a notification to the app's notification context.
Parameters:
- notification_type: The type of notification (e.g., 'success', 'error').
- message: The message to display in the notification.
- ctx: The context to use for the notification. Defaults to Dash's callback context if not provided.
- nid: An optional notification ID.
"""
# Use the provided context or default to Dash's callback context
context = ctx or dash._callback_context
# Safely retrieve the current children from the context
try:
updated_props = context._get_context_value().updated_props
children = updated_props.get('app_notification', {}).get('children', [])
except AttributeError:
# Handle cases where context or updated_props might be None
children = []
# Append the new notification
children.append(notification(notification_type, message, ctx, nid))
# Update the properties with the new list of children
set_props("app_notification", {"children": children})
Updating this would require that I pull the props for show, update, hide and update them as needed, whereas having the notification provide the action is easier from this standpoint.
|
Also note that Mantine is releasing V8 next month. If the best solution has breaking changes, we could include them with the others when we upgrade to V8. |
|
The notifications wont be breaking changes as this is a new component. We should really deprecate the old version though. |
…determine the action by a key in the notification itself, like the original api from DMC
|
|
||
| // Define the Notification interface based on your requirements | ||
| interface Notification { | ||
| color?: MantineColor; |
There was a problem hiding this comment.
- Need to add docstrings for each prop.
- Missing
style,styles,className,classNamesalthough currently dash does not do any error checking and it's possible to include these props anyway, but it would be good to add for completeness. - If there is an invalid option used for
positionthere is a hard to debug error message generated by Mantine. We should validate thepositionprop before it's sent to Mantine.
| /** Notifications container z-index, `400` by default */ | ||
| zIndex?: string | number; | ||
| /** Determines whether notifications container should be rendered inside `Portal`, `true` by default */ | ||
| withinPortal?: boolean; |
There was a problem hiding this comment.
missing the portalProps prop.
Also, do we need the store prop?
There was a problem hiding this comment.
store as in what? Is this to be from the notifications? If so, we'd have to have a hook from the store to know when it updated.
There was a problem hiding this comment.
Ok, let's skip the store prop
|
|
||
| const componentPath = getContextPath() | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Is there a reason for using useEffect rather than Mantine's UseDidUpdate ? https://mantine.dev/hooks/use-did-update/
There was a problem hiding this comment.
useDidUpdate only runs on the prop updates, this would not capture sendNotifications initially sent to the component.
Co-authored-by: Ann Marie Ward <72614349+AnnMarieW@users.noreply.github.com>
…r.tsx Co-authored-by: Ann Marie Ward <72614349+AnnMarieW@users.noreply.github.com>
|
Should add a couple tests for |
|
Here are the new Notification docs snehilvj/dmc-docs#199 |
|
@alexcjohnson This PR is ready for a final review. We are planning on including this in V2 so that people can refer to the V1 docs if they are still using the deprecated components. The new Notification docs and a detailed migration guide are ready (with some final edits to do) The only open item is whether to rename the component |
| const { setProps, loading_state, ...others } = props; | ||
|
|
||
| useEffect(() => { | ||
| console.warn('This method of Notifications is deprecated and will be removed in a future major release. Instead, use `NotificationContainer') |
There was a problem hiding this comment.
| console.warn('This method of Notifications is deprecated and will be removed in a future major release. Instead, use `NotificationContainer') | |
| console.warn('`NotificationProvider` is deprecated and will be removed in a future major release. Instead, use `NotificationContainer`') |
Might not be obvious from the console what "this method" is (and "method" has a more precise meaning anyway, ie a function bound to a class)
There was a problem hiding this comment.
Good point - that's better. But should probably be NotificationProvider and Notification are deprecated....
alexcjohnson
left a comment
There was a problem hiding this comment.
LGTM! I'd stick with NotificationContainer, I see the point about it holding multiple notifications, but it's grammatically funny. A shoe store sells many shoes, a bike lane can have many bikes...
utilizes the api more closely (https://mantine.dev/x/notifications/)
exposes the api and store at
appNotificationsin thedash_mantine_componentsnamespacefixes #525