Update PortfolioSummary.tsx #938
Conversation
|
@GradleD is attempting to deploy a commit to the LFG Labs Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in Changes
Possibly related PRs
Suggested reviewers
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: 1
🧹 Outside diff range and nitpick comments (2)
components/dashboard/PortfolioSummary.tsx (2)
99-101: Consider making the loading skeleton height responsiveThe fixed height of 300px might not be optimal for all screen sizes. Consider using relative units or min/max heights for better responsiveness.
- <Loading isLoading={loading} loadingType="skeleton" width="100%" height={300}> + <Loading isLoading={loading} loadingType="skeleton" width="100%" height="min(300px, 50vh)">
Line range hint
99-154: Improve component structure with early returns and error handlingThe current nested conditional structure could be simplified, and error states should be handled in the UI.
Consider restructuring the component's return statement:
return ( <Loading isLoading={loading} loadingType="skeleton" width="100%" height={300}> + {error && ( + <div className="text-red-500"> + <Typography type={TEXT_TYPE.BODY_MIDDLE}> + {error.message} + </Typography> + </div> + )} - {data.length > 0 ? ( + {!error && data.length === 0 && ( + <Typography type={TEXT_TYPE.BODY_MIDDLE}> + No portfolio data available + </Typography> + )} + {!error && data.length > 0 && ( <div className={styles.dashboard_portfolio_summary}> {/* ... existing content ... */} </div> - ) : null} + )} </Loading> );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/dashboard/PortfolioSummary.tsx(5 hunks)
🔇 Additional comments (2)
components/dashboard/PortfolioSummary.tsx (2)
1-1: LGTM: Import statements are properly organized
The new imports for React hooks and the Loading component are correctly placed and follow good practices.
Also applies to: 13-13
Line range hint 1-157: Overall implementation successfully addresses the skeleton loading requirement
The changes effectively implement skeleton loading for the PortfolioSummary component, addressing the original PR objective. The implementation follows React best practices and provides a good user experience during loading states.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| setLoading(false); | ||
| }; | ||
| fetchData(); | ||
| }, []); |
There was a problem hiding this comment.
What is this? Why are you simulating a loading? Are you to understand the issue ?
| @@ -85,7 +96,10 @@ const PortfolioSummary: FunctionComponent<PortfolioSummaryProps> = ({ title, dat | |||
| const [showClaimModal, setShowClaimModal] = useState(false); | |||
| const [showSuccessModal, setShowSuccessModal] = useState(false); | |||
|
|
|||
| return data.length > 0 ? ( | |||
| return ( | |||
| <Loading isLoading={loading} loadingType="skeleton" width="100%" height={300}> | |||
There was a problem hiding this comment.
Why are you doing this ? We want to make a skeleton loading for this part not rendering it randomly
| @@ -135,7 +149,9 @@ const PortfolioSummary: FunctionComponent<PortfolioSummaryProps> = ({ title, dat | |||
| </div> | |||
| </div> | |||
| </div> | |||
| ) : null; | |||
| ) : null} | |||
| </Loading> | |||
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
components/dashboard/PortfolioSummary.tsx (1)
Line range hint
99-154: Refine skeleton loading implementationThe current implementation wraps the entire component with skeleton loading, but based on the reviewer's screenshot, we should only apply skeleton loading to specific parts of the UI.
Consider this approach:
return ( - <Loading isLoading={loading} loadingType="skeleton" width="100%" height={300}> {data.length > 0 ? ( <div className={styles.dashboard_portfolio_summary}> <div className="flex flex-col md:flex-row w-full justify-between items-center mb-4"> {/* ... header content ... */} </div> <div className={styles.dashboard_portfolio_summary_info}> <div className="flex flex-col justify-between w-10/12 md:w-8/12 h-fit"> + {!isDataLoaded ? ( + <Loading loadingType="skeleton" width="100%" height={24} count={3} className="mb-2" /> + ) : ( {data.map((item, id) => ( <ChartEntry key={id} {...item} /> ))} + )} </div> <div className="w-full mb-4 md:w-3/12 md:mb-0"> + {!isDataLoaded ? ( + <Loading loadingType="skeleton" width="100%" height="100%" className="rounded-full" /> + ) : ( <Doughnut data={{ labels: data.map(entry => entry.itemLabel), datasets: [{/* ... */}], }} options={chartOptions} /> + )} </div> </div> </div> ) : null} - </Loading> );This approach:
- Removes the wrapper Loading component
- Adds targeted skeleton loading for specific UI elements
- Shows skeleton UI only while actual data is loading
|
Hey @GradleD please update this or I'll assign to someone else |
Summary by CodeRabbit
New Features
Bug Fixes