style: resolve fade on scroll issue on accounts list #1968
Conversation
WalkthroughReplaces ActionContext navigation with React Router's useNavigate in BodySection, adds a VelvetBox prop Changes
Sequence Diagram(s)sequenceDiagram
participant UI as AccountsList UI
participant Body as BodySection.tsx
participant Router as React Router
rect rgb(220,235,255)
UI->>Body: user action / accounts removed
Body->>Body: compute flatAccounts === []
alt empty accounts
Body->>Router: navigate('/')
Router-->>UI: route -> root
else non-empty
Body-->>UI: render accounts list
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/extension-polkagate/src/popup/accountsLists/BodySection.tsx(4 hunks)packages/extension-polkagate/src/popup/accountsLists/index.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (2)
packages/extension-polkagate/src/popup/accountsLists/index.tsx (1)
76-76: LGTM - Overflow clipping supports fade effect.Adding
overflow: 'hidden'to the Grid container appropriately clips overflow content, which is essential for the fade-on-scroll effect to work correctly.packages/extension-polkagate/src/popup/accountsLists/BodySection.tsx (1)
107-107: VelvetBox correctly accepts thechildrenStyleprop—no issues found.VelvetBox's interface (VelvetBoxProp) defines
childrenStyle?: React.CSSProperties(line 24 in packages/extension-polkagate/src/style/VelvetBox.tsx), and the component properly destructures it with a default value. The usage in BodySection.tsx passingborderRadiusandoverflowis valid and correctly typed.
| import { Box, Container, Stack } from '@mui/material'; | ||
| import { AddCircle, Trash } from 'iconsax-react'; | ||
| import React, { memo, useCallback, useContext, useEffect, useMemo, useRef, useState } from 'react'; | ||
| import { useNavigate } from 'react-router-dom'; |
There was a problem hiding this comment.
Critical: useNavigate is not available in react-router-dom v5.3.3.
The project uses react-router-dom v5.3.3, but useNavigate is a v6+ API. This import will fail at runtime.
Based on learnings.
Use the v5 equivalent instead:
-import { useNavigate } from 'react-router-dom';
+import { useHistory } from 'react-router-dom';Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/extension-polkagate/src/popup/accountsLists/BodySection.tsx around
line 9, the code imports useNavigate which is a react-router-dom v6 API and will
fail at runtime with v5.3.3; replace the import with useHistory from
'react-router-dom' and update any navigation calls to use history.push(...) or
history.replace(...) accordingly (obtain history via const history =
useHistory() and use history.push/replace where useNavigate() would have been
used).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/extension-polkagate/src/popup/accountsLists/BodySection.tsx (3)
9-9: Critical:useNavigateis not available in react-router-dom v5.3.3.This issue was previously flagged and remains unresolved. The project uses react-router-dom v5.3.3, which does not support the
useNavigatehook (v6+ API). This will cause a runtime error.Based on learnings.
Replace with the v5 equivalent:
-import { useNavigate } from 'react-router-dom'; +import { useHistory } from 'react-router-dom';
52-52: Critical: ReplaceuseNavigatewithuseHistory.This issue was previously flagged and remains unresolved. Use the v5-compatible hook:
Based on learnings.
- const navigate = useNavigate(); + const history = useHistory();
68-70: Critical: Update navigation to use v5 API.This issue was previously flagged and remains unresolved. The navigation logic must use the v5
history.push()method, and theas voidcast is unnecessary.Based on learnings.
useEffect(() => { if (flatAccounts.length === 0) { - navigate('/') as void; + history.push('/'); } - }, [flatAccounts.length, navigate]); + }, [flatAccounts.length, history]);
🧹 Nitpick comments (1)
packages/extension-polkagate/src/popup/accountsLists/AccountRowSimple.tsx (1)
60-60: Verify the margin-to-padding change doesn't affect layout.Changing from
mx: '1px'(horizontal margin) topx: '1px'(horizontal padding) alters the box model. This could affect the element's total width and click target area. Please verify that this change produces the intended visual result and doesn't introduce layout shifts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/extension-polkagate/src/popup/accountsLists/AccountRowSimple.tsx(1 hunks)packages/extension-polkagate/src/popup/accountsLists/BodySection.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
🔇 Additional comments (3)
packages/extension-polkagate/src/popup/accountsLists/AccountRowSimple.tsx (1)
57-57: Animation refinement looks good.The reduced initial offset (10 → 5) creates a more subtle entrance animation.
packages/extension-polkagate/src/popup/accountsLists/BodySection.tsx (2)
107-107: Verify VelvetBox supports thechildrenStyleprop.The new
childrenStyleprop withoverflow: 'hidden'aligns well with the PR objective to fix the fade-on-scroll issue. Please confirm that the VelvetBox component's API includes this prop to avoid runtime errors.
108-108: Height constraints and scroll behavior look appropriate.The explicit height constraints with
overflowY: 'auto'enable the scrollable area that theFadeOnScrollcomponent (line 156) operates on. This aligns with the PR objective.
2 participants
fixes the fade on scroll issue on accounts list (extension)
Summary by CodeRabbit
Bug Fixes
Refactor
Style