feat(notificationstack): New hooks and components for NotificationStack #12572
Conversation
|
DCO Assistant Lite bot All contributors have signed the DCO. |
✅ Deploy Preview for carbon-components-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
I have read the DCO document and I hereby sign the DCO. |
|
@cgirani could you just post the message without the quoted text in a new comment? Thanks! |
There was a problem hiding this comment.
Thanks so much for submitting this! It's a great first step towards landing something like this in the project.
I'd love to schedule some time to review this with you further and see how we could nail down the experience and API a bit more.
Some high level thoughts from an initial glance:
NotificationStack- Component to be used in begin of the application, it's a wrapper of all notifications
Great approach here, I think it's a very common pattern to expect consumers to wrap part or all of their application tree with a context-provider-type of component like this.
notification-store- Function where we hold the state of notifications, along with functions that act like dispatch/reducer
I'd like to see this use useReducer instead of a vanilla reducer pattern. Also, have you ran into any issues storing these in state? I could forsee a situation where someone needs to persist the notification store across sessions - we might want to think about how to open up this to enable "saving" to an external store, if even just localstorage.
notification- Export a function to trigger the notification, this function can be imported in any file in the application
I'd like to see this provided by the hook, instead of it's own function to import. Also we should look into using useLayoutEffect instead of a setTimeout for being able to calculate the height.
useNotification- Hook with notifications info from the store and some handlers (actions/dispatch) to be used, this one is used in the component NotificationStack
I'd really like to refocus this on the experience of the consumer. I think in an ideal world we'd have the useNotification hook as a central entrypoint for all of this behavior. trigger would be destructured off the hook in consumer apps or in storybook examples.
Will a consumer ever need to dispatch manually? If we need a second hook for the internal dispatch, etc., that would be fine, but we should probably keep those concerns separate from useNotification if there's no need for a consumer to call dispatch.
Additional thoughts:
- This only works with ToastNotification. Ideally, InlineNotification and ActionableNotification support would be included and provide the animation defined in the root issue.
- I see you haven't added any of this to the root index.js exports yet. We'll want to really hone in on what we intend to expose to consumers and think about limiting that API surface area so the underlying implementation can shift without introducing breaking changes to consumers.
- As this firms up we can first introduce it as experimental, prefixed as
unstable__useNotification. - Respecting the user's
prefers-reduced-motionsetting will be important to provide support out of the gate
Again thank you for this initial drop! 🙏 🎉 I can reach out to schedule some time to discuss this.
|
I have read the DCO document and I hereby sign the DCO. |
|
Sorry this has languished - our team has had other priorities and been unable to pick up this workstream and devote the time and attention it deserves. If anyone is willing to continue this work based on the suggestions and feedback provided above, we'd love to chat. Going to close this for now to clean up our PR queue, but happy to reopen if work resumes |
Reference: #8405
New hooks and components to render NotificationStack.
Components/hooks and their use/purpose:
NotificationStack- Component to be used in begin of the application, it's a wrapper of all notificationsnotification-store- Function where we hold the state of notifications, along with functions that act like dispatch/reducernotification- Export a function to trigger the notification, this function can be imported in any file in the applicationuseNotification- Hook with notifications info from the store and some handlers (actions/dispatch) to be used, this one is used in the componentNotificationStackEven files are aligned about Notifications hooks, trigger function and store are generic enough for future uses of any other component...
Changelog
New
Changed
Index.cssto add new css filesRemoved
Testing / Reviewing
At the storybook test the NotificationStack component.