style: shake account icons once on activation #1603
Conversation
WalkthroughThis pull request introduces several modifications across multiple files. The Changes
Possibly related PRs
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: 5
🧹 Outside diff range and nitpick comments (2)
packages/extension-polkagate/src/fullscreen/accountDetails/components/useAnimateOnce.tsx (1)
6-7: Consider validating the condition parameterThe condition parameter accepts undefined but the implementation doesn't handle this case explicitly. Consider adding validation or documenting the behavior.
-export default function useAnimateOnce (condition: boolean | undefined): boolean { +export default function useAnimateOnce (condition: boolean): boolean { const [animate, setAnimate] = useState(false);packages/extension-polkagate/src/fullscreen/accountDetails/components/AccountIconsFs.tsx (1)
103-119: Consider using optional chaining for API queries.While the bracket notation works, optional chaining might be more readable for API queries.
-if (api.query?.['recovery'] && SOCIAL_RECOVERY_CHAINS.includes(account.genesisHash)) { +if (api.query?.recovery && SOCIAL_RECOVERY_CHAINS.includes(account.genesisHash)) { api.query.recovery.recoverable(formatted)Also, consider extracting the type assertions into helper functions for better maintainability:
const parseProxies = (result: unknown): Proxy[] => { const [announcements] = result as [Vec<PalletProxyAnnouncement>, u128]; return JSON.parse(JSON.stringify(announcements)) as Proxy[]; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/AccountIconsFs.tsx (7 hunks)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/useAnimateOnce.tsx (1 hunks)
- packages/extension/public/locales/en/translation.json (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/extension/public/locales/en/translation.json
🧰 Additional context used
🪛 Biome
packages/extension-polkagate/src/fullscreen/accountDetails/components/AccountIconsFs.tsx
[error] 10-10: Do not shadow the global "Proxy" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (2)
packages/extension-polkagate/src/fullscreen/accountDetails/components/useAnimateOnce.tsx (1)
1-4: LGTM: Proper license and importsThe file includes the correct license header and necessary React imports.
packages/extension-polkagate/src/fullscreen/accountDetails/components/AccountIconsFs.tsx (1)
39-41: LGTM! Clean hook implementation for animations.The animation states are properly separated for each icon type, following React best practices.
packages/extension-polkagate/src/fullscreen/accountDetails/components/useAnimateOnce.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/accountDetails/components/useAnimateOnce.tsx
Outdated
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/accountDetails/components/AccountIconsFs.tsx
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/accountDetails/components/AccountIconsFs.tsx
Show resolved
Hide resolved
packages/extension-polkagate/src/fullscreen/accountDetails/components/AccountIconsFs.tsx
Show resolved
Hide resolved
* refactor: optimize useAnimateOnce * refactor: remove unused --------- Co-authored-by: Nick <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
packages/extension-polkagate/src/hooks/useAnimateOnce.tsx (1)
6-17: Consider defining defaults in the interface.While the current implementation is correct, consider moving the default values into the interface definition for better maintainability and type documentation.
interface AnimateOnceConfig { - duration?: number; - delay?: number; + duration?: number; // defaults to 500 + delay?: number; // defaults to 0 onStart?: () => void; onComplete?: () => void; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- packages/extension-polkagate/src/fullscreen/accountDetails/components/AccountIconsFs.tsx (8 hunks)
- packages/extension-polkagate/src/hooks/index.ts (1 hunks)
- packages/extension-polkagate/src/hooks/useAnimateOnce.tsx (1 hunks)
🧰 Additional context used
🪛 Biome
packages/extension-polkagate/src/fullscreen/accountDetails/components/AccountIconsFs.tsx
[error] 10-10: Do not shadow the global "Proxy" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (7)
packages/extension-polkagate/src/hooks/useAnimateOnce.tsx (2)
1-4: LGTM! Imports and header are properly structured.The file includes appropriate copyright notice, license information, and necessary React hook imports.
53-59: LGTM! Effect cleanup is properly implemented.The useEffect hook properly handles cleanup on unmount and dependency changes.
packages/extension-polkagate/src/fullscreen/accountDetails/components/AccountIconsFs.tsx (4)
18-18: LGTM! Good performance optimization.The component is now properly memoized using
React.memo, which can help prevent unnecessary re-renders.Also applies to: 27-27, 187-188
38-40: LGTM! Animation states are properly managed.The
useAnimateOncehook is correctly applied to manage the shake animations for each icon.
102-105: LGTM! Improved type safety and API access.Good improvements:
- Using bracket notation for API queries is more robust
- Type assertions properly handle the query results
Also applies to: 111-115
144-144: LGTM! Animation props are properly implemented.The shake animations are correctly bound to their respective states through the FontAwesomeIcon components.
Also applies to: 149-149, 167-167, 178-178
packages/extension-polkagate/src/hooks/index.ts (1)
16-16: LGTM! Verify the hook implementation.The export statement follows the established patterns in the file and is correctly placed in alphabetical order.
Let's verify that the hook implementation exists:
✅ Verification successful
Let me gather more information about the hook implementation since we found the file but need to verify its contents.
The previous results confirmed the file exists but with a
.tsxextension. Let's examine its contents to ensure it exports a default function and follows proper hook implementation patterns before making a final decision.
Hook implementation verified and follows best practices
The hook implementation exists and is well-implemented with:
- Proper TypeScript types and interfaces
- Accessibility consideration (prefers-reduced-motion)
- Proper cleanup of timeouts
- Default exports as expected
- Comprehensive JSDoc documentation
- Follows React hooks best practices (useCallback, useEffect, useRef)
The export in the barrel file correctly matches this implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the useAnimateOnce hook implementation exists and exports a default. # Test 1: Check if the hook file exists fd -t f "useAnimateOnce.ts" -x echo "Hook file found: {}" # Test 2: Verify the hook exports a default ast-grep --pattern 'export default function useAnimateOnce'Length of output: 196
Script:
#!/bin/bash # Check the actual implementation of useAnimateOnce hook cat ./packages/extension-polkagate/src/hooks/useAnimateOnce.tsxLength of output: 1924
| export default function useAnimateOnce (condition: boolean | undefined, config: AnimateOnceConfig = {}): boolean { | ||
| const [animate, setAnimate] = useState(false); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider stricter typing for the condition parameter.
The condition parameter accepts undefined, which might lead to unexpected behavior. Consider making it strictly boolean.
-export default function useAnimateOnce (condition: boolean | undefined, config: AnimateOnceConfig = {}): boolean {
+export default function useAnimateOnce (condition: boolean, config: AnimateOnceConfig = {}): boolean {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default function useAnimateOnce (condition: boolean | undefined, config: AnimateOnceConfig = {}): boolean { | |
| const [animate, setAnimate] = useState(false); | |
| export default function useAnimateOnce (condition: boolean, config: AnimateOnceConfig = {}): boolean { | |
| const [animate, setAnimate] = useState(false); |
| const cleanup = useCallback(() => { | ||
| if (timeoutRef.current) { | ||
| clearTimeout(timeoutRef.current); | ||
| timeoutRef.current = undefined; | ||
| } | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance cleanup implementation.
The cleanup function should also reset the animation state to ensure consistent behavior during unmount or when dependencies change.
const cleanup = useCallback(() => {
if (timeoutRef.current) {
clearTimeout(timeoutRef.current);
timeoutRef.current = undefined;
+ setAnimate(false);
}
}, []);Committable suggestion was skipped due to low confidence.
| // Check if the user prefers reduced motion | ||
| const prefersReducedMotion = window.matchMedia('(prefers-reduced-motion: reduce)').matches; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility check implementation.
The current implementation checks motion preferences only once during initialization. Consider using a more robust solution that can react to preference changes.
-const prefersReducedMotion = window.matchMedia('(prefers-reduced-motion: reduce)').matches;
+const mediaQuery = window.matchMedia('(prefers-reduced-motion: reduce)');
+const [prefersReducedMotion, setPrefersReducedMotion] = useState(mediaQuery.matches);
+
+useEffect(() => {
+ const handler = (event: MediaQueryListEvent) => {
+ setPrefersReducedMotion(event.matches);
+ };
+
+ mediaQuery.addEventListener('change', handler);
+ return () => mediaQuery.removeEventListener('change', handler);
+}, []);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check if the user prefers reduced motion | |
| const prefersReducedMotion = window.matchMedia('(prefers-reduced-motion: reduce)').matches; | |
| // Check if the user prefers reduced motion | |
| const mediaQuery = window.matchMedia('(prefers-reduced-motion: reduce)'); | |
| const [prefersReducedMotion, setPrefersReducedMotion] = useState(mediaQuery.matches); | |
| useEffect(() => { | |
| const handler = (event: MediaQueryListEvent) => { | |
| setPrefersReducedMotion(event.matches); | |
| }; | |
| mediaQuery.addEventListener('change', handler); | |
| return () => mediaQuery.removeEventListener('change', handler); | |
| }, []); |
| import type { DeriveAccountInfo } from '@polkadot/api-derive/types'; | ||
| import type { Option, u128, Vec } from '@polkadot/types'; | ||
| //@ts-ignore | ||
| import type { PalletProxyAnnouncement, PalletRecoveryActiveRecovery } from '@polkadot/types/lookup'; |
There was a problem hiding this comment.
Fix type shadowing and improve type imports.
The imported Proxy type shadows the global Proxy object, which could lead to confusion. Consider renaming it to be more specific.
-import type { Proxy } from '../../../util/types';
+import type { AccountProxy } from '../../../util/types';
// Update usage of Proxy type to AccountProxy throughout the file
-const fetchedProxies = JSON.parse(JSON.stringify(_p[0])) as unknown as Proxy[];
+const fetchedProxies = JSON.parse(JSON.stringify(_p[0])) as unknown as AccountProxy[];Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
New Features
useAnimateOnce, for managing one-time animations based on conditions.Bug Fixes
Refactor
AccountIconsFscomponent and updated function signatures for clarity.