-
Notifications
You must be signed in to change notification settings - Fork 1
feat: redesign member cards with expandable roles/KPIs section #366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add expandable section showing RoleCard and KpiCard components - Simplify bar chart legend with new simpleLegend prop - Add legend spacing to pie chart - Update team section with LayoutGrid icon and 'Teams:' label - Rename GoalsRadarChart to GoalsBarChart for accuracy - Improve text hierarchy and uniform spacing
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded@drifter089 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 3 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
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 |
PR Review: Redesign Member CardsSummaryThis PR successfully redesigns the team member cards with improved styling, an expandable section for roles and KPIs, and a simplified legend for charts. The implementation is generally solid with good attention to detail. ✅ Strengths
🔍 Issues & Concerns1. Type Safety Concern in roleToCardData() (member-card.tsx:50-66)The function roleToCardData(role: Role): RoleCardData {
return {
// ... other fields
assignedUserName: null, // ⚠️ Always null - loses potential data
// ...
};
}Recommendation: Check if the Role type includes user name information and preserve it if available. 2. Potential Layout Shift on Expand (member-card.tsx:254-316)The expandable section renders role and KPI data that could be quite large. If a member has many roles/KPIs, expanding could cause significant layout shifts. Recommendation: Consider adding a max height with scroll for the expanded content or implementing pagination/virtualization for large lists. 3. Empty State Handling in Goals ChartWhen 4. Skeleton Height Mismatch (member-card.tsx:212-213)The skeleton states show <Skeleton className="h-[320px] w-full" /> // ⚠️ Should be h-[340px]Fix: Update skeleton heights to 5. Missing Error HandlingThe component doesn't handle errors from the Recommendation: Add error handling: const { data: roles, isLoading, isError } = api.role.getByUser.useQuery({
userId: member.id,
});
if (isError) {
// Show error state
}🎨 UI/UX Suggestions1. Improve Teams Section AccessibilityThe team badges are clickable but lack visual affordance. Consider adding an icon or hover state to make it clearer they're interactive. 2. Loading State for Expanded ContentWhen expanding the section, the roles are already loaded from the query, but KPIs might not be. Consider showing individual loading states for the KPI cards if needed. 3. Animation for CollapsibleThe 🐛 Potential Bugs1. Null Safety in KPI Filtering (member-card.tsx:98-107)The filter assumes const memberKpis = useMemo(() => {
if (!roles || dashboardCharts.length === 0) return [];
const memberMetricIds = new Set(
roles.filter((r) => r.metricId != null).map((r) => r.metricId!),
);
return dashboardCharts.filter((chart) =>
memberMetricIds.has(chart.metric.id), // ⚠️ What if chart.metric.id is undefined?
);
}, [roles, dashboardCharts]);Recommendation: Add null check for 2. Empty teamId Prop to KpiCard (member-card.tsx:306)Passing an empty string as <KpiCard
key={chart.id}
dashboardChart={chart}
teamId={chart.metric.teamId ?? ""} // ⚠️ Empty string fallback
showSettings={false}
enableDragDrop={false}
/>Recommendation: Handle the null case more gracefully, perhaps by not rendering the card or showing a warning. 📊 Performance Considerations
🧪 Testing Recommendations
🔒 Security ConsiderationsNo significant security concerns identified. The component properly handles user input and doesn't introduce XSS vulnerabilities. 📝 Code QualityPositives:
Minor Improvements:
Overall AssessmentScore: 7.5/10 This is a solid PR that improves the user experience with better visual design and additional functionality. The main concerns are around edge case handling and minor type safety issues. With the recommended fixes, this would be production-ready. Required Changes:
Suggested Improvements:
Great work overall! The expandable section is a nice UX improvement that keeps the interface clean while providing access to detailed information. 🎉 |
- Add expandable section showing RoleCard and KpiCard components - Simplify bar chart legend with new simpleLegend prop - Add legend spacing to pie chart - Update team section with LayoutGrid icon and 'Teams:' label - Rename GoalsRadarChart to GoalsBarChart for accuracy - Improve text hierarchy and uniform spacing
Summary
Redesigns the team member cards on the /member page with improved styling, simplified chart legends, and an expandable section showing roles and KPI cards.
Key Changes