Notification toast #581
Notification toast #581
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a structured notification system across multiple components in the application. It adds a Changes
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/router/src/guardian-ui/setup/FederationSetup.tsxOops! Something went wrong! :( ESLint: 8.40.0 Error: Cannot read config file: /apps/router/.eslintrc.js
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
apps/router/src/home/NotificationProvider.tsx (1)
15-69: Consider making notification defaults configurableThe component has hardcoded values for duration (5000ms) and position ('top-right'). Consider accepting these as props with defaults to make the provider more flexible for different use cases.
-export const NotificationProvider: React.FC<React.PropsWithChildren> = ({ +interface NotificationProviderProps extends React.PropsWithChildren { + defaultDuration?: number; + defaultPosition?: UseToastOptions['position']; +} + +export const NotificationProvider: React.FC<NotificationProviderProps> = ({ children, + defaultDuration = 5000, + defaultPosition = 'top-right', }) => { // ... const showNotification = useCallback( (message: string, status: UseToastOptions['status'], options?: UseToastOptions) => { toast({ description: message, status, - duration: 5000, + duration: defaultDuration, isClosable: true, - position: 'top-right', + position: defaultPosition, ...options, }); }, - [toast] + [toast, defaultDuration, defaultPosition] );apps/router/src/guardian-ui/components/setup/RunDKG.tsx (1)
Line range hint
1-18: Add missing notification importsThe notification functions (
showInfo,showSuccess) are used but not imported. Add the required imports from the notification provider.Add this import:
import { useTranslation } from '@fedimint/utils'; +import { useNotification } from '../../../providers/NotificationProvider'; import { useEllipsis } from '../../hooks';Then initialize the notification hooks at the start of the component:
const { showInfo, showSuccess } = useNotification();apps/router/src/gateway-ui/components/federations/ConnectFederation.tsx (1)
Line range hint
171-171: Fix modal onClose handler implementationThe onClose prop should be a function that checks the loading state, not a boolean expression.
- <ConnectFedModal isOpen={loading} onClose={() => !loading} /> + <ConnectFedModal isOpen={loading} onClose={() => loading ? undefined : onClose()} />apps/router/src/guardian-ui/components/setup/screens/verifyGuardians/VerifyGuardians.tsx (1)
154-155: Consider combining success notificationsTwo consecutive notifications (success and info) might overwhelm the user. Consider combining these into a single success message.
- showSuccess(t('verify-guardians.verified')); - showInfo(t('verify-guardians.all-guardians-verified')); + showSuccess(t('verify-guardians.verified') + '. ' + t('verify-guardians.all-guardians-verified'));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
apps/router/src/gateway-ui/components/federations/ConnectFederation.tsx(3 hunks)apps/router/src/guardian-ui/components/setup/RunDKG.tsx(1 hunks)apps/router/src/guardian-ui/components/setup/screens/connectGuardians/ConnectGuardians.tsx(2 hunks)apps/router/src/guardian-ui/components/setup/screens/setConfiguration/SetConfiguration.tsx(1 hunks)apps/router/src/guardian-ui/components/setup/screens/verifyGuardians/VerifyGuardians.tsx(4 hunks)apps/router/src/guardian-ui/setup/FederationSetup.tsx(4 hunks)apps/router/src/home/NotificationProvider.tsx(1 hunks)apps/router/src/index.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/router/src/guardian-ui/components/setup/screens/setConfiguration/SetConfiguration.tsx
🧰 Additional context used
📓 Learnings (1)
apps/router/src/guardian-ui/setup/FederationSetup.tsx (1)
Learnt from: Kodylow
PR: fedimint/ui#570
File: apps/router/src/guardian-ui/setup/FederationSetup.tsx:23-23
Timestamp: 2024-11-12T07:32:55.886Z
Learning: In `apps/router/src/guardian-ui/setup/FederationSetup.tsx`, the hooks `useGuardianSetupApi` and `useGuardianSetupContext` are correctly imported from `'../../hooks'`.
🔇 Additional comments (7)
apps/router/src/home/NotificationProvider.tsx (3)
1-9: LGTM: Well-structured interface definition
The interface properly defines the notification methods with correct typing for messages and options.
11-13: LGTM: Proper context initialization
The undefined initial value ensures proper provider usage.
71-79: LGTM: Well-implemented custom hook
The hook follows React best practices and provides clear error messages.
apps/router/src/guardian-ui/components/setup/screens/connectGuardians/ConnectGuardians.tsx (1)
27-27: LGTM! Clean notification integration.
The notification system integration is well-implemented:
- Proper hook usage and dependency management
- Notification is shown before navigation
- Translations are correctly utilized
Also applies to: 34-34, 60-62
apps/router/src/guardian-ui/components/setup/screens/verifyGuardians/VerifyGuardians.tsx (3)
44-44: LGTM: Notification hooks properly integrated
The NotificationProvider import and hook usage are correctly implemented.
Also applies to: 73-73
170-173: LGTM: Dependencies properly declared
The dependency arrays for both useEffect and handleNext are correctly updated to include the notification functions.
Also applies to: 187-187
180-180: Ensure error handling is comprehensive
The error notification is shown after setting the error state. Consider adding more specific error messages for different failure scenarios.
Also applies to: 184-184
apps/router/src/gateway-ui/components/federations/ConnectFederation.tsx
Outdated
Show resolved
Hide resolved
apps/router/src/gateway-ui/components/federations/ConnectFederation.tsx
Outdated
Show resolved
Hide resolved
|
@kleyberthsantos can you run through these coderabbit suggestions before I review? |
|
@Kodylow I will review Coderabbit's suggestions and let you know when it is ready. Thanks! |
|
@Kodylow, all CodeRabbit suggestions have been addressed:
Ready for review 🚀 |
|
I share a sample of the functionality of notifications. UI_notification.mp4 |
|
can you remove that package-lock.json otherwise good to go |
-Yes, already on that.- @Kodylow, package-lock.json has been removed ✅ |
| <ChakraProvider> | ||
| <SharedChakraProvider theme={theme}> |
There was a problem hiding this comment.
I don't think we want to add an additional ChakraProvider. We should be using the existing SharedChakraProvider.
Is there additional rationale for adding this? Are you running into any issues with the existing provider?
There was a problem hiding this comment.
Hi @alexlwn123 👋
The SharedChakraProvider is mainly designed to share the theme and base styles across components, while the root ChakraProvider handles specific functionalities like the notification system (toasts).
I suggest keeping both providers for now and creating a separate issue to explore how we could unify them without breaking the notification functionality. Let me know what you think!
There was a problem hiding this comment.
Could you please provide more details on the issues you're facing with the existing SharedChakraProvider?
There was a problem hiding this comment.
The issue with the existing SharedChakraProvider lies in its current implementation, which doesn’t configure the toastOptions required for the notification system.
-
NotificationProvider Dependency:
- The
NotificationProvideruses theuseToasthook from Chakra UI, which requires the full Chakra context, including the internalToastProvider.
- The
-
SharedChakraProvider Limitation:
- While the
SharedChakraProviderextends theChakraProvider, it does not currently pass thetoastOptionsprop, which is necessary to configure notification defaults (e.g., position, duration, etc.).
- While the
-
Resulting Issue:
- Due to this limitation, the notifications fail to display entirely, as the
useToasthook cannot access the required context provided by theToastProvider.
- Due to this limitation, the notifications fail to display entirely, as the
-
Technical Details:
- The
ChakraProvidernatively supports atoastOptionsprop as part of itsChakraProviderProps. However, our current implementation ofSharedChakraProviderdoes not utilize this feature, causing the notification system to break.
- The
| duration: 5000, | ||
| isClosable: true, | ||
| position: 'top-right', | ||
| ...options, |
There was a problem hiding this comment.
Would it make sense to add these defaults to the Chakra theme? If so, should we reference the useToast directly in components instead of adding an additional provider?
ref: https://v2.chakra-ui.com/docs/components/toast/usage#configuring-toast-globally
There was a problem hiding this comment.
For now, it wouldn’t be optimal to move these defaults to the Chakra theme. While it makes sense conceptually, in practice, configuring toasts through the theme causes notifications to appear in the center of the screen instead of the top-right, even when the position is explicitly specified.
There was a problem hiding this comment.
Thanks for clarifying! I haven’t seen an issue where configuring toasts globally forces them to the center. Could you share a snippet or repro?
There was a problem hiding this comment.
Here’s a snippet of the configuration when moving the toast settings to theme.tsx:
packages/ui/src/theme.tsx
Toast: {
defaultProps: {
duration: 5000,
isClosable: true,
position: 'top-right'
}
}
Here's a screenshot showing how notifications appear in the center and bottom instead of in the top right corner:
Issue #496
Implementation Summary
Objective: Implement a unified notification system using Chakra UI's toast functionality to ensure a consistent user experience and improve code maintainability.
Key Changes:
NotificationProvider: Created to centralize and manage notifications (success, error, warning, info) using Chakra UI's
useToast.Updated Components:
Features:
Provider Implementation:
SharedChakraProvider.ChakraProviderwithNotificationProviderfor full toast functionality.Summary by CodeRabbit
Release Notes
New Features
NotificationProviderfor managing notifications throughout the application.Bug Fixes
These updates aim to enhance user interaction and provide clearer feedback during key processes.