feat: add profile tab tooltip in FS #1567
Conversation
+ removing alerts after shown from context
WalkthroughThe pull request introduces several modifications across multiple components in the codebase. Key changes include updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 4
🧹 Outside diff range and nitpick comments (2)
packages/extension-polkagate/src/hooks/useAlerts.ts (1)
17-19: Approve with suggestion: Add error handling to removeAlertThe
removeAlertfunction is well-implemented usinguseCallbackfor memoization and follows React's immutable state update pattern. However, consider adding a check for invalid indices to prevent unexpected behavior.Consider adding an index check:
const removeAlert = useCallback((index: number) => { + if (index < 0 || index >= alerts.length) { + console.warn(`Attempted to remove alert at invalid index: ${index}`); + return; + } setAlerts((prev) => prev.filter((_, i) => i !== index)); }, [setAlerts]);packages/extension-polkagate/src/components/Infotip2.tsx (1)
91-91: LGTM: Style prop reordering.The reordering of style properties doesn't affect functionality. However, for better consistency and maintainability, consider adopting a standard order for style properties across the project.
You might want to consider using a consistent order for style properties throughout the project. For example, you could order them alphabetically or group them by type (layout, spacing, typography, etc.). This can make it easier to scan and maintain styles across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- packages/extension-polkagate/src/components/Infotip2.tsx (2 hunks)
- packages/extension-polkagate/src/components/contexts.tsx (1 hunks)
- packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/ProfileTabFullScreen.tsx (3 hunks)
- packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/ProfileTabsFullScreen.tsx (1 hunks)
- packages/extension-polkagate/src/hooks/useAlerts.ts (1 hunks)
- packages/extension-polkagate/src/partials/AlertBox.tsx (1 hunks)
- packages/extension/public/locales/en/translation.json (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/ProfileTabsFullScreen.tsx
- packages/extension/public/locales/en/translation.json
🔇 Additional comments (16)
packages/extension-polkagate/src/hooks/useAlerts.ts (1)
11-11: LGTM: Accessing alerts from contextThe modification to destructure
alertsalong withsetAlertsfrom the context is a good change. It allows the hook to access the current list of alerts, which is necessary for the new functionality and for exposing the alerts in the return value.packages/extension-polkagate/src/components/contexts.tsx (2)
Line range hint
1-54: Summary: Minor import change with no functional impact.The only change in this file is the import of the
noopfunction from@polkadot/utilinstead of a local utility file. This change does not affect the functionality of the contexts defined in this file. The overall structure and purpose of the file remain intact.
11-11: Approved: Good practice to use standard library function.The change from a local
noopimplementation to the one provided by@polkadot/utilis a good practice. It promotes consistency and leverages a well-maintained library.To ensure consistency across the project, let's verify if there are any remaining usages of the local
noopfunction:If this script returns any results, consider updating those instances to use the
@polkadot/utilimport as well.packages/extension-polkagate/src/components/Infotip2.tsx (2)
22-22: LGTM: Improved function signature formatting.The addition of a space after the function name in the signature improves readability and aligns with common coding conventions.
Line range hint
1-138: Summary: Minor stylistic improvements with no functional changes.The changes in this file are purely stylistic and don't affect the component's functionality. The Infotip2 component's logic, structure, and behavior remain unchanged. These improvements enhance code readability without introducing any risks or requiring additional testing.
packages/extension-polkagate/src/fullscreen/homeFullScreen/partials/ProfileTabFullScreen.tsx (7)
13-13: LGTM: Import statement updated correctly.The import statement has been appropriately updated to include
Infotip2, which is used later in the component.
31-31: LGTM: Expanded useAlerts hook usage.The
useAlertshook now correctly includesalerts, which is used later in the component for conditional rendering. This change aligns with the PR objectives regarding alert handling improvements.
87-93: LGTM: Infotip2 wrapper implemented correctly.The
Infotip2component has been appropriately added to wrap the existingGridcomponent, implementing the profile tab tooltip feature as per the PR objectives. The conditional logic for the tooltip text is well-structured, ensuring it's displayed only under specific conditions (no alerts, selected tab, and not all accounts hidden).
94-119: LGTM: Grid component props enhanced for better layout and styling.The
Gridcomponent's props have been updated to improve layout, alignment, and styling. The additions ofalignItems,columnGap, andjustifyContentprops, along with the expandedsxobject, contribute to a more polished and responsive UI. These changes enhance the overall user experience without introducing any apparent issues.
120-120: LGTM: Checkmark icon implemented correctly.The
VaadinIconcomponent is appropriately used to display a checkmark, with its visibility conditionally set based on theisSelectedprop. This implementation provides clear visual feedback for the selected profile tab, enhancing the user experience.
121-133: LGTM: Typography component styling enhanced.The
Typographycomponent has been updated with improved styling, including better handling of text overflow and smooth visibility transitions. These changes enhance the display of profile names, especially for longer text, and improve the overall responsiveness of the UI. The implementation aligns well with best practices for text display in limited space.
134-142: LGTM: Eye-slash icon implemented correctly.The new
VaadinIconcomponent for the eye-slash icon is well-implemented. Its visibility and appearance are appropriately conditioned on theareAllHiddenandvisibleContentstates, providing clear visual feedback about the hidden state of profile accounts. This addition enhances the user interface and aligns with the PR objectives regarding profile account visibility management.packages/extension-polkagate/src/partials/AlertBox.tsx (4)
9-9: ImportinguseEffectfrom React is appropriateThe addition of
useEffectis necessary for managing side effects related to alerts.
11-11: Verify the necessity ofuseTransactionStateWhile
useAlertsis used, please confirm whetheruseTransactionStateis required in this component, as it doesn't appear in the provided code.
14-15: Good practice: Define a constant for alert removal timeDefining
TIME_TO_REMOVE_ALERTas a constant enhances readability and maintainability.
17-17: Destructuring alerts and removeAlertUsing object destructuring for
alertsandremoveAlertfromuseAlertshook is concise and clear.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
packages/extension-polkagate/src/partials/Alert.tsx (1)
23-25: LGTM: Alert closure logic updated and optimized.The
closeAlertfunction has been optimized usinguseCallbackand now directly removes the alert using theremoveAlertfunction from theuseAlertshook. This change aligns with the new alert management approach described in the AI summary.Consider moving the
useCallbackdependency array to a new line for better readability:const closeAlert = useCallback(() => { removeAlert(alert.id); - }, [alert.id, removeAlert]); + }, [ + alert.id, + removeAlert + ]);packages/extension-polkagate/src/hooks/useAlerts.ts (2)
11-11: LGTM: Alert removal time constant addedThe addition of
TIME_TO_REMOVE_ALERTconstant is good for maintaining consistency in alert removal timing. The 5-second duration seems reasonable for most alerts.Consider making this value configurable, either through a prop or a configuration object, to allow for flexibility in different use cases.
Line range hint
1-32: Overall: Excellent enhancements to alert managementThe changes to
useAlerts.tssignificantly improve the alert management system:
- Unique IDs for alerts using the Chance library ensure proper tracking and removal of alerts.
- Automatic alert removal after a set time improves user experience.
- The expanded interface (
alerts,notify,removeAlert) provides more control to consumers of the hook.- Proper use of React hooks optimizes performance.
These enhancements result in a more robust, efficient, and user-friendly alert system. Great job on the improvements!
Consider the following for future iterations:
- Make the
TIME_TO_REMOVE_ALERTconfigurable for different use cases.- Implement a mechanism to pause the auto-removal timer when the user interacts with an alert, if applicable to your use case.
packages/extension-polkagate/src/hooks/useAssetsBalances.ts (1)
140-141: LGTM: Chance instance created and addAlert function implemented.The Chance instance is correctly memoized, and the addAlert function is well-implemented. It generates unique IDs for alerts, sets them with a success message, and removes them after a specified duration.
Consider extracting the alert message to a constant or configuration file for easier maintenance and potential internationalization:
+const ACCOUNTS_BALANCE_UPDATED_MESSAGE = 'Accounts\' balances updated!'; const addAlert = useCallback(() => { const id = random.string({ length: 10 }); - setAlerts((perv) => [...perv, { id, severity: 'success', text: t('Accounts\' balances updated!') }]); + setAlerts((perv) => [...perv, { id, severity: 'success', text: t(ACCOUNTS_BALANCE_UPDATED_MESSAGE) }]); const timeout = setTimeout(() => setAlerts((prev) => prev.filter(({ id: alertId }) => alertId !== id)), TIME_TO_REMOVE_ALERT); return () => clearTimeout(timeout); }, [random, setAlerts, t]);Also applies to: 154-161
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- packages/extension-polkagate/src/hooks/useAlerts.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useAssetsBalances.ts (5 hunks)
- packages/extension-polkagate/src/partials/Alert.tsx (1 hunks)
- packages/extension-polkagate/src/partials/AlertBox.tsx (1 hunks)
- packages/extension-polkagate/src/util/types.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/extension-polkagate/src/partials/AlertBox.tsx
🔇 Additional comments (11)
packages/extension-polkagate/src/partials/Alert.tsx (4)
11-11: LGTM: Import changes align with new functionality.The addition of
useCallbackanduseAlertsimports are consistent with the changes in alert handling logic described in the AI summary. These imports support the new implementation for managing alerts.Also applies to: 13-13
28-28: LGTM: Slide transition always active.The
inprop of theSlidecomponent is now always set totrue, which aligns with the removal of the visibility state management mentioned in the AI summary. This ensures that the alert is always rendered with the slide transition.To ensure this change doesn't affect the alert's disappearance animation, please verify the following:
- Check if the
Slidecomponent handles unmounting correctly when the alert is removed.- Confirm that the
unmountOnExitprop ensures proper cleanup after the exit animation.You may want to test this behavior in the UI to ensure smooth transitions for both appearance and disappearance of alerts.
21-21: LGTM: Alert removal now managed through useAlerts hook.The
removeAlertfunction from theuseAlertshook is now used for alert management, which aligns with the changes described in the AI summary. This approach centralizes alert handling logic.Let's verify the implementation of the
useAlertshook:
Line range hint
1-41: Overall: Good improvements to alert handling and component optimization.The changes in this file align well with the PR objectives and the AI summary. The alert handling has been centralized using the
useAlertshook, and the component has been optimized withuseCallback. These modifications improve code organization while maintaining existing functionality.To ensure full compatibility with the rest of the application, please verify:
- The
useAlertshook implementation and its usage across the application.- The behavior of alerts in different scenarios (e.g., multiple alerts, rapid alert creation/removal).
Run the following script to check for other uses of alert-related functions:
This will help ensure that the changes are consistent throughout the application.
✅ Verification successful
Alert Functions Usage Verified
The
useAlerts,removeAlert, andaddAlertfunctions are utilized across multiple files, includingAlertBox.tsx,OptionalCopyPopup.tsx, and others. The changes inAlert.tsxeffectively centralize alert handling and optimize the component without introducing issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other uses of alert-related functions # Test: Search for uses of removeAlert and other alert-related functions rg --type typescript -A 5 'removeAlert|addAlert|useAlerts'Length of output: 95
Script:
#!/bin/bash # Description: Check for other uses of alert-related functions with correct TypeScript file types # Test: Search for uses of removeAlert and other alert-related functions in .ts and .tsx files rg --type ts -A 5 'removeAlert|addAlert|useAlerts' rg --type tsx -A 5 'removeAlert|addAlert|useAlerts'Length of output: 14516
Script:
#!/bin/bash # Description: Check for other uses of alert-related functions in .ts and .tsx files using glob patterns # Test: Search for uses of removeAlert, addAlert, and useAlerts in .ts and .tsx files rg 'removeAlert|addAlert|useAlerts' --glob '*.ts' --glob '*.tsx' -A 5Length of output: 14453
packages/extension-polkagate/src/hooks/useAlerts.ts (2)
6-6: LGTM: Chance library import addedThe addition of the Chance library import is appropriate for generating unique IDs for alerts. This change supports the new functionality in the hook.
16-29: LGTM: Improved alert management in useAlerts hookThe changes to the
useAlertshook implementation are well-structured and improve alert management:
- The use of
useMemofor the Chance instance optimizes performance.- The
removeAlertfunction is correctly implemented withuseCallback.- The
notifyfunction now generates unique IDs and schedules alert removal, enhancing alert management.- The use of
setTimeoutfor alert removal with a proper cleanup function is correct.These changes provide a more robust and efficient alert system.
packages/extension-polkagate/src/util/types.ts (1)
785-785: Approved: Addition ofidproperty enhances alert managementThe addition of the
id: string;property to theAlertTypeinterface is a positive change. This unique identifier will facilitate better management and tracking of individual alerts, aligning with the PR objective of removing alerts after they've been displayed. It's a non-breaking change that enhances the interface's functionality without disrupting existing code.packages/extension-polkagate/src/hooks/useAssetsBalances.ts (4)
12-12: LGTM: Chance library import added.The Chance library is appropriately imported for generating random strings, which will be used for creating unique alert IDs.
23-23: LGTM: TIME_TO_REMOVE_ALERT constant imported.Importing the TIME_TO_REMOVE_ALERT constant from the useAlerts hook ensures consistency in alert duration across the application.
222-225: LGTM: Alert added after account saving.The addAlert function is correctly called after handleAccountsSaving, ensuring that users are notified when account balances are successfully updated.
Line range hint
1-577: Overall assessment: Changes improve user feedback and code quality.The modifications to this file enhance the user experience by providing notifications when account balances are updated. The implementation is solid, with good use of React hooks and proper management of side effects. The suggestion to extract the alert message to a constant is a minor improvement that could be considered for better maintainability.
Summary by CodeRabbit
Release Notes
New Features
removeAlertmethod for better control over alert visibility.Bug Fixes
ProfileTabFullScreencomponent to improve the display of contextual tooltips.Style
ProfileTabsFullScreencomponent for improved visual spacing.Refactor
useAlertshook to provide additional functionality for managing alerts and streamline alert handling in theAlertBoxcomponent.