refactor(dashboards): add filters and carousel layout #147
Conversation
- Extract chart options to shared constants (SPARKLINE_CHART_OPTIONS, BAR_CHART_OPTIONS) - Refactor toSignal initializations to private initializer methods - Add filter pills component for metric categorization - Implement horizontal carousel with scroll controls - Display dynamic account name in section header - Remove unused getOrganizationSegmentOverview code across all layers - Remove function description comments from private methods LFXV2-717 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Asitha de Silva <asithade@gmail.com>
github-actions
bot
requested review from
andrest50,
dealako and
mauriciozanettisalomao
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRedesigned Organization Involvement UI to a horizontal carousel with filter pills and unified chart rendering. Removed segment/sponsorship-related APIs, types, routes, and service methods across backend and frontend. Added FilterPills component and new chart options; updated docs and commit guideline to include running yarn build. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant UI as OrganizationInvolvementComponent
participant Pills as FilterPillsComponent
participant Analytics as AnalyticsService (client)
participant Server as AnalyticsController (server)
participant OrgSvc as OrganizationService (server)
note right of UI `#DDEBF7`: Carousel + filter state
User->>Pills: click filter pill
Pills-->>UI: emit filterChange(filterId)
UI->>UI: handleFilterChange(filterId)\nupdate selectedFilter -> recompute primaryMetrics
UI->>Analytics: request metrics (contributions/events/education)
Analytics->>Server: GET /analytics/organization-events-overview (or contributions)
Server->>OrgSvc: fetch aggregated data (events, contributors, maintainers, etc.)
OrgSvc-->>Server: return eventAttendance / contributions
Server-->>Analytics: response (eventAttendance, contributors, maintainers)
Analytics-->>UI: provide data signals
UI->>UI: transform data -> primaryMetrics\nrender charts in carousel
User->>UI: click carousel left/right
UI->>UI: scrollLeft()/scrollRight() -> adjust carouselScrollContainer.scrollLeft
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the organization involvement dashboard component by improving code organization, adding filtering capabilities, and implementing a horizontal carousel layout for better mobile experience.
Key Changes:
- Extracted chart configuration constants (
SPARKLINE_CHART_OPTIONS,BAR_CHART_OPTIONS) to shared constants for reusability - Added filter pills component for categorizing metrics by All, Contributions, Events, and Education
- Implemented horizontal scrollable carousel with navigation controls replacing the previous grid-based layout
- Removed unused "Event Sponsorship" and segment overview functionality across all application layers
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/shared/src/interfaces/dashboard.interface.ts | Removed unused interfaces for contribution and impact metrics along with isConnected property |
| packages/shared/src/interfaces/analytics-data.interface.ts | Removed unused interfaces for segment contributions and event sponsorships data |
| packages/shared/src/constants/organization-involvement.constants.ts | Extracted chart options to constants and added new metrics (Event Attendees, Event Speakers, Certified Employees, Training Enrollments) |
| docs/architecture/frontend/state-management.md | Updated documentation examples to use public instead of protected access modifiers |
| docs/architecture/frontend/angular-patterns.md | Updated documentation examples to use public instead of protected access modifiers |
| apps/lfx-one/src/server/services/organization.service.ts | Removed getSegmentOverview method and event sponsorships logic from events overview |
| apps/lfx-one/src/server/routes/analytics.route.ts | Removed unused segment overview endpoint |
| apps/lfx-one/src/server/controllers/analytics.controller.ts | Removed getOrganizationSegmentOverview controller method |
| apps/lfx-one/src/app/shared/services/analytics.service.ts | Removed getOrganizationSegmentOverview service method and event sponsorships handling |
| apps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.ts | New reusable filter pills component for metric categorization |
| apps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.html | Template for filter pills with active state styling |
| apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.ts | Major refactor: extracted signal initializations to methods, added filtering logic, carousel controls, and new metric transformers |
| apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.scss | Added CSS utility class to hide scrollbar for carousel |
| apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.html | Replaced grid layout with horizontal carousel, added filter pills and scroll controls |
| CLAUDE.md | Added reminder to run yarn build for validation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.ts
Outdated
Show resolved
Hide resolved
apps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.html
Show resolved
Hide resolved
...modules/dashboards/components/organization-involvement/organization-involvement.component.ts
Show resolved
Hide resolved
...modules/dashboards/components/organization-involvement/organization-involvement.component.ts
Show resolved
Hide resolved
![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
🧹 Nitpick comments (4)
apps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.html (1)
6-16: Consider adding accessibility attributes for better UX.The filter pills lack ARIA attributes to communicate their state to assistive technologies. Consider adding:
aria-pressedto indicate selected statearia-labelfor more descriptive button labelsrole="group"on the container witharia-label="Filter options"Apply this diff to improve accessibility:
-<div class="flex items-center gap-2"> +<div class="flex items-center gap-2" role="group" aria-label="Filter options"> @for (option of options(); track option.id) { <button type="button" (click)="handleFilterChange(option.id)" + [attr.aria-pressed]="selectedFilter() === option.id" + [attr.aria-label]="option.label + ' filter'" [class]=" selectedFilter() === option.id ? 'px-3 py-1 rounded-full text-sm transition-colors bg-[#0094FF] text-white' : 'px-3 py-1 rounded-full text-sm transition-colors bg-slate-100 text-slate-700 hover:bg-slate-200' " [attr.data-testid]="'filter-pill-' + option.id"> {{ option.label }} </button> } </div>apps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.ts (1)
7-10: Consider moving FilterOption interface to shared package.The
FilterOptioninterface is a simple, reusable type that could benefit other components. Per coding guidelines, consider moving it topackages/shared/src/interfaces/components.interface.tsfor better reusability across the monorepo.As per coding guidelines.
apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.html (1)
43-98: Consider adding keyboard navigation for carousel.The carousel scroll buttons work well for mouse users, but users navigating with keyboard would benefit from arrow key support to scroll through metrics. This would improve accessibility and user experience.
Consider adding a keydown handler in the component to handle arrow key navigation:
@HostListener('keydown', ['$event']) public handleKeyboardNavigation(event: KeyboardEvent): void { if (event.key === 'ArrowLeft') { event.preventDefault(); this.scrollLeft(); } else if (event.key === 'ArrowRight') { event.preventDefault(); this.scrollRight(); } }Also ensure the carousel container has
tabindex="0"to be keyboard focusable:- <div #carouselScroll class="flex gap-4 overflow-x-auto pb-2 hide-scrollbar scroll-smooth" data-testid="dashboard-involvement-carousel"> + <div #carouselScroll class="flex gap-4 overflow-x-auto pb-2 hide-scrollbar scroll-smooth" data-testid="dashboard-involvement-carousel" tabindex="0" role="region" aria-label="Organization metrics carousel">apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.ts (1)
300-307: Guard against missing sparkline colors
hexToRgbathrows when it receives an empty string. Because each transformer callshexToRgba(metric.sparklineColor ?? '', 0.1), any future metric without asparklineColor(or one left blank) will crash the component. Please guard before invokinghexToRgbaand apply the same fix to the other transformers using this pattern.- backgroundColor: hexToRgba(metric.sparklineColor ?? '', 0.1), + backgroundColor: metric.sparklineColor ? hexToRgba(metric.sparklineColor, 0.1) : 'transparent',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
CLAUDE.md(1 hunks)apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.html(3 hunks)apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.scss(1 hunks)apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.ts(2 hunks)apps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.html(1 hunks)apps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.ts(1 hunks)apps/lfx-one/src/app/shared/services/analytics.service.ts(0 hunks)apps/lfx-one/src/server/controllers/analytics.controller.ts(0 hunks)apps/lfx-one/src/server/routes/analytics.route.ts(0 hunks)apps/lfx-one/src/server/services/organization.service.ts(1 hunks)docs/architecture/frontend/angular-patterns.md(2 hunks)docs/architecture/frontend/state-management.md(4 hunks)packages/shared/src/constants/organization-involvement.constants.ts(3 hunks)packages/shared/src/interfaces/analytics-data.interface.ts(1 hunks)packages/shared/src/interfaces/dashboard.interface.ts(0 hunks)
💤 Files with no reviewable changes (4)
- apps/lfx-one/src/server/controllers/analytics.controller.ts
- apps/lfx-one/src/app/shared/services/analytics.service.ts
- apps/lfx-one/src/server/routes/analytics.route.ts
- packages/shared/src/interfaces/dashboard.interface.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript interfaces instead of union types for better maintainability
When defining PrimeNG-related types, reference the official PrimeNG component interfaces
Files:
apps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.tspackages/shared/src/interfaces/analytics-data.interface.tsapps/lfx-one/src/server/services/organization.service.tsapps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.tspackages/shared/src/constants/organization-involvement.constants.ts
**/*.{ts,tsx,js,jsx,mjs,cjs,html,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
apps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.tsapps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.scsspackages/shared/src/interfaces/analytics-data.interface.tsapps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.htmlapps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.htmlapps/lfx-one/src/server/services/organization.service.tsapps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.tspackages/shared/src/constants/organization-involvement.constants.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.tspackages/shared/src/interfaces/analytics-data.interface.tsapps/lfx-one/src/server/services/organization.service.tsapps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.tspackages/shared/src/constants/organization-involvement.constants.ts
packages/shared/src/interfaces/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place all TypeScript interfaces in the shared package at packages/shared/src/interfaces
Files:
packages/shared/src/interfaces/analytics-data.interface.ts
apps/lfx-one/src/**/*.html
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-one/src/**/*.html: Always add data-testid attributes when creating new Angular components for reliable test targeting
Use data-testid naming convention [section]-[component]-[element]
Files:
apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.htmlapps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.html
packages/shared/src/constants/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place all reusable constants in the shared package at packages/shared/src/constants
Files:
packages/shared/src/constants/organization-involvement.constants.ts
🧠 Learnings (5)
📚 Learning: 2025-09-16T03:32:46.518Z
Learnt from: CR
Repo: linuxfoundation/lfx-v2-ui PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T03:32:46.518Z
Learning: All PrimeNG components are wrapped in LFX components to keep UI library independence
Applied to files:
apps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.ts
📚 Learning: 2025-09-16T03:32:46.518Z
Learnt from: CR
Repo: linuxfoundation/lfx-v2-ui PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T03:32:46.518Z
Learning: Applies to apps/lfx-one/src/**/*.html : Always add data-testid attributes when creating new Angular components for reliable test targeting
Applied to files:
apps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.html
📚 Learning: 2025-09-16T03:32:46.518Z
Learnt from: CR
Repo: linuxfoundation/lfx-v2-ui PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T03:32:46.518Z
Learning: Pre-commit hooks enforce license headers; run lint and format before build; prepend 'Generated with Claude Code' if assisted
Applied to files:
CLAUDE.md
📚 Learning: 2025-09-16T03:32:46.518Z
Learnt from: CR
Repo: linuxfoundation/lfx-v2-ui PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T03:32:46.518Z
Learning: Follow Angular conventional commits with JIRA ticket association and PR/title formatting rules
Applied to files:
CLAUDE.md
📚 Learning: 2025-10-21T21:19:13.599Z
Learnt from: andrest50
Repo: linuxfoundation/lfx-v2-ui PR: 125
File: apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts:345-350
Timestamp: 2025-10-21T21:19:13.599Z
Learning: In the Angular meeting card component (apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts), when selecting between `summary.summary_data.edited_content` and `summary.summary_data.content`, the logical OR operator (`||`) is intentionally used instead of nullish coalescing (`??`) because empty string edited_content should fall back to the original content rather than being displayed as empty.
Applied to files:
apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.ts
🧬 Code graph analysis (3)
apps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.ts (1)
apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.ts (1)
Component(17-414)
apps/lfx-one/src/server/services/organization.service.ts (1)
packages/shared/src/interfaces/analytics-data.interface.ts (1)
OrganizationEventAttendanceRow(192-198)
apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.ts (4)
apps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.ts (2)
Component(12-26)FilterOption(7-10)packages/shared/src/constants/organization-involvement.constants.ts (3)
SPARKLINE_CHART_OPTIONS(20-28)BAR_CHART_OPTIONS(34-50)PRIMARY_INVOLVEMENT_METRICS(57-112)packages/shared/src/interfaces/dashboard.interface.ts (2)
OrganizationInvolvementMetricWithChart(93-132)PrimaryInvolvementMetric(64-87)packages/shared/src/utils/color.utils.ts (1)
hexToRgba(28-56)
⏰ 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). (1)
- GitHub Check: Agent
🔇 Additional comments (12)
packages/shared/src/interfaces/analytics-data.interface.ts (1)
336-339: LGTM! Clear documentation update.The updated comment accurately reflects that this interface now represents attendance data only, aligning with the removal of event sponsorship functionality.
apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.scss (1)
4-11: LGTM! Well-implemented cross-browser utility.The
.hide-scrollbarclass provides comprehensive cross-browser support for hiding scrollbars, which is appropriate for the carousel UI.packages/shared/src/constants/organization-involvement.constants.ts (3)
20-28: LGTM! Standard Chart.js sparkline configuration.The sparkline chart options are correctly configured for minimal trend visualizations with disabled interactivity.
34-50: LGTM! Appropriate bar chart configuration.The bar chart options provide good defaults for compact metric visualizations with proper bar styling.
57-112: TODO comments appropriately flag placeholder data.The dummy sparkline data generated by
generateTrendDatais clearly marked with TODO comments indicating it should be replaced with API trend data. This is a good practice for configuration-only constants.docs/architecture/frontend/angular-patterns.md (1)
64-66: LGTM! Documentation reflects updated visibility patterns.The change from protected to public for template-accessed properties aligns with Angular best practices and the project's move toward explicit public APIs.
apps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.ts (1)
12-26: LGTM! Well-structured standalone component.The component follows Angular signals patterns correctly with input signals and output events. The implementation is clean and type-safe.
apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.html (3)
5-11: LGTM! Clean filter integration with dynamic header.The dynamic account name and filter pills component are well-integrated with proper data binding and event handling.
14-31: LGTM! Accessible carousel controls.The scroll controls include proper
data-testidattributes following the convention andaria-labelattributes for accessibility.
78-86: Avoid nested ternary expressions in template.The chart configuration uses nested ternary expressions to determine both chart type and options, which violates the coding guideline against nested ternaries. This reduces readability and makes the template harder to maintain.
As per coding guidelines.
Consider computing the chart type and options in the component TypeScript:
In the component, add computed properties or methods:
public getChartType(metric: PrimaryInvolvementMetric): 'bar' | 'line' { return metric.title === 'Active Contributors' || metric.title === 'Maintainers' ? 'bar' : 'line'; } public getChartOptions(metric: PrimaryInvolvementMetric) { return metric.title === 'Active Contributors' || metric.title === 'Maintainers' ? this.barChartOptions : this.sparklineChartOptions; }Then simplify the template:
- <lfx-chart - [type]="metric.title === 'Active Contributors' || metric.title === 'Maintainers' ? 'bar' : 'line'" - [data]="metric.chartData" - [options]="metric.title === 'Active Contributors' || metric.title === 'Maintainers' ? barChartOptions : sparklineChartOptions" - height="100%"></lfx-chart> + <lfx-chart + [type]="getChartType(metric)" + [data]="metric.chartData" + [options]="getChartOptions(metric)" + height="100%"></lfx-chart>⛔ Skipped due to learnings
Learnt from: CR Repo: linuxfoundation/lfx-v2-ui PR: 0 File: CLAUDE.md:0-0 Timestamp: 2025-09-16T03:32:46.518Z Learning: Applies to **/*.{ts,tsx,js,jsx} : Do not nest ternary expressionsLearnt from: andrest50 Repo: linuxfoundation/lfx-v2-ui PR: 125 File: apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts:345-350 Timestamp: 2025-10-21T21:19:13.599Z Learning: In the Angular meeting card component (apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts), when selecting between `summary.summary_data.edited_content` and `summary.summary_data.content`, the logical OR operator (`||`) is intentionally used instead of nullish coalescing (`??`) because empty string edited_content should fall back to the original content rather than being displayed as empty.docs/architecture/frontend/state-management.md (2)
121-121: LGTM! Documentation reflects public service injection pattern.The visibility change from protected to public for injected services aligns with modern Angular patterns and improves template access clarity.
153-174: LGTM! Consistent public API for state and actions.The updated visibility for state signals and action methods provides a clear, public API surface that aligns with the project's architectural direction.
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-147.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
Add safety checks to prevent runtime errors when carousel scroll buttons are clicked before ViewChild is initialized or when conditionally rendered. LFXV2-717 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Asitha de Silva <asithade@gmail.com>
Reverse the order of pending actions and my meetings in board member dashboard to show pending actions on the left and my meetings on the right. LFXV2-717 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Asitha de Silva <asithade@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.ts (2)
108-118: LGTM: Null checks properly address previous review concern.The scroll controls now include proper null guards, preventing potential runtime errors if called before view initialization. This successfully addresses the previous review feedback.
371-392: The hardcoded training enrollments value must be replaced with API data.Line 374 still uses a hardcoded value of '156' for training enrollments, which was flagged in a previous review. This displays incorrect data to users and should be replaced with actual API data or marked with a TODO comment indicating it's placeholder data pending API implementation.
Apply this diff to add a TODO comment at minimum:
private transformTrainingEnrollments(metric: PrimaryInvolvementMetric): OrganizationInvolvementMetricWithChart { return { title: metric.title, - value: '156', + value: '156', // TODO: Replace with actual API data for training enrollments subtitle: 'Employees enrolled in training',
🧹 Nitpick comments (2)
apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.ts (2)
49-102: Consider refactoring the metric transformation dispatch logic.The if-chain pattern for dispatching to transformation methods works but could be more maintainable. Consider using a map-based approach for better scalability:
private readonly transformerMap = new Map<string, (data: any, metric: PrimaryInvolvementMetric) => OrganizationInvolvementMetricWithChart>([ ['Active Contributors', (_, metric) => this.transformActiveContributors(this.contributionsOverviewData().contributors, metric)], ['Maintainers', (_, metric) => this.transformMaintainers(this.contributionsOverviewData().maintainers, metric)], ['Event Attendees', (_, metric) => this.transformEventAttendees(this.eventsOverviewData().eventAttendance, metric)], // ... etc ]); public readonly primaryMetrics = computed<OrganizationInvolvementMetricWithChart[]>(() => { const allMetrics = PRIMARY_INVOLVEMENT_METRICS.map((metric) => { const transformer = this.transformerMap.get(metric.title); if (transformer) return transformer(null, metric); if (metric.isMembershipTier) return this.transformMembershipTier(this.boardMemberDashboardData().membershipTier, metric); return this.transformDefaultMetric(metric); }); // ... filtering logic });
205-415: Consider extracting common chart data construction logic.Multiple transformation methods contain nearly identical chartData construction logic (lines 214-227, 237-250, 302-315, 328-341, etc.). This duplication could be reduced by extracting a helper method:
private createChartData( metric: PrimaryInvolvementMetric, useTransparentBackground = false ): { labels: string[]; datasets: any[] } { return { labels: Array.from({ length: metric.sparklineData?.length ?? 0 }, (_, i) => `Point ${i + 1}`), datasets: [ { data: metric.sparklineData ?? [], borderColor: metric.sparklineColor ?? '', backgroundColor: useTransparentBackground ? hexToRgba(metric.sparklineColor ?? '', 0.1) : metric.sparklineColor ?? '', fill: true, tension: 0.4, borderWidth: 2, pointRadius: 0, }, ], }; }Then update transformation methods to use:
chartData: this.createChartData(metric, true)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/lfx-one/src/app/layouts/main-layout/main-layout.component.ts(1 hunks)apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.ts(2 hunks)apps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript interfaces instead of union types for better maintainability
When defining PrimeNG-related types, reference the official PrimeNG component interfaces
Files:
apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.tsapps/lfx-one/src/app/layouts/main-layout/main-layout.component.ts
**/*.{ts,tsx,js,jsx,mjs,cjs,html,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.tsapps/lfx-one/src/app/layouts/main-layout/main-layout.component.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.tsapps/lfx-one/src/app/layouts/main-layout/main-layout.component.ts
🧠 Learnings (1)
📚 Learning: 2025-10-21T21:19:13.599Z
Learnt from: andrest50
Repo: linuxfoundation/lfx-v2-ui PR: 125
File: apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts:345-350
Timestamp: 2025-10-21T21:19:13.599Z
Learning: In the Angular meeting card component (apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts), when selecting between `summary.summary_data.edited_content` and `summary.summary_data.content`, the logical OR operator (`||`) is intentionally used instead of nullish coalescing (`??`) because empty string edited_content should fall back to the original content rather than being displayed as empty.
Applied to files:
apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.ts
🧬 Code graph analysis (1)
apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.ts (4)
apps/lfx-one/src/app/shared/components/filter-pills/filter-pills.component.ts (2)
Component(12-26)FilterOption(7-10)packages/shared/src/constants/organization-involvement.constants.ts (3)
SPARKLINE_CHART_OPTIONS(20-28)BAR_CHART_OPTIONS(34-50)PRIMARY_INVOLVEMENT_METRICS(57-112)packages/shared/src/interfaces/dashboard.interface.ts (2)
OrganizationInvolvementMetricWithChart(93-132)PrimaryInvolvementMetric(64-87)packages/shared/src/utils/color.utils.ts (1)
hexToRgba(28-56)
⏰ 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). (1)
- GitHub Check: build-and-push
🔇 Additional comments (5)
apps/lfx-one/src/app/layouts/main-layout/main-layout.component.ts (2)
48-48: Verify that disabling the Documentation link is intentional.The Documentation menu item is now disabled, preventing users from accessing the documentation site. While this may be intentional (e.g., documentation is under construction), it's worth confirming:
- Is this change intentional and does it relate to the dashboard refactoring objectives?
- Should there be a tracking issue or timeline for re-enabling documentation access?
1-89: LGTM: Well-structured Angular component.The component follows Angular best practices with proper dependency injection, reactive patterns using RxJS, and appropriate use of TypeScript types. The code is clean, type-safe, and maintainable.
apps/lfx-one/src/app/modules/dashboards/components/organization-involvement/organization-involvement.component.ts (3)
1-23: LGTM: Component setup and imports are well-structured.The license header is present, and the imports correctly include the new FilterPillsComponent and shared constants needed for the refactored carousel layout.
24-47: LGTM: State management setup is clean and follows Angular best practices.Good use of signals, computed properties, and the ViewChild pattern. The filter options correctly support the new categorization (All, Contributions, Events, Education) as specified in the PR objectives.
120-203: LGTM: Data initialization methods follow a consistent and robust pattern.The three initialization methods properly handle:
- Loading state management with finalize
- Proper initial values matching API response shapes
- Reactive data flow triggered by account changes
The consistent pattern makes the code maintainable and predictable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/lfx-one/src/app/modules/dashboards/board-member/board-member-dashboard.component.html (1)
29-35: Component reordering looks good; consider adding data-testid attributes to improve test targeting.The grid structure is maintained correctly with proper responsive behavior (grid-cols-1 md:grid-cols-2) and spacing (gap-6). The reordering of My Meetings and Pending Actions aligns with the stated UI reorganization.
For consistency with the coding guideline about data-testid attributes and to improve E2E test reliability, consider adding data-testid attributes to the individual component tags (e.g.,
data-testid="dashboard-pending-actions"anddata-testid="dashboard-my-meetings"). This would provide more granular test targeting across the dashboard, consistent with the pattern used on container elements.Apply this diff to add component-level data-testid attributes:
<!-- Pending Actions --> - <lfx-pending-actions class="h-full" /> + <lfx-pending-actions class="h-full" data-testid="dashboard-pending-actions" /> <!-- My Meetings --> - <lfx-my-meetings class="h-full" /> + <lfx-my-meetings class="h-full" data-testid="dashboard-my-meetings" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/lfx-one/src/app/modules/dashboards/board-member/board-member-dashboard.component.html(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
apps/lfx-one/src/**/*.html
📄 CodeRabbit inference engine (CLAUDE.md)
apps/lfx-one/src/**/*.html: Always add data-testid attributes when creating new Angular components for reliable test targeting
Use data-testid naming convention [section]-[component]-[element]
Files:
apps/lfx-one/src/app/modules/dashboards/board-member/board-member-dashboard.component.html
**/*.{ts,tsx,js,jsx,mjs,cjs,html,css,scss}
📄 CodeRabbit inference engine (CLAUDE.md)
Include required license headers on all source files
Files:
apps/lfx-one/src/app/modules/dashboards/board-member/board-member-dashboard.component.html
🧠 Learnings (1)
📚 Learning: 2025-10-21T21:19:13.599Z
Learnt from: andrest50
Repo: linuxfoundation/lfx-v2-ui PR: 125
File: apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts:345-350
Timestamp: 2025-10-21T21:19:13.599Z
Learning: In the Angular meeting card component (apps/lfx-one/src/app/modules/project/meetings/components/meeting-card/meeting-card.component.ts), when selecting between `summary.summary_data.edited_content` and `summary.summary_data.content`, the logical OR operator (`||`) is intentionally used instead of nullish coalescing (`??`) because empty string edited_content should fall back to the original content rather than being displayed as empty.
Applied to files:
apps/lfx-one/src/app/modules/dashboards/board-member/board-member-dashboard.component.html
⏰ 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). (1)
- GitHub Check: build-and-push
🔇 Additional comments (1)
apps/lfx-one/src/app/modules/dashboards/board-member/board-member-dashboard.component.html (1)
1-2: License header is properly in place. ✓
🧹 Deployment RemovedThe deployment for PR #147 has been removed. |
Summary
Refactors the organization involvement component with improved code organization and enhanced UI features.
Changes
JIRA
https://linuxfoundation.atlassian.net/browse/LFXV2-717
Technical Details
SPARKLINE_CHART_OPTIONSandBAR_CHART_OPTIONSto@lfx-one/shared/constantsFilterPillsComponentfor metric filteringinitializeContributionsOverviewData(),initializeBoardMemberDashboardData(),initializeEventsOverviewData()SegmentContributionsConsolidatedRow,OrganizationSegmentOverviewResponseTesting
🤖 Generated with Claude Code