-
Notifications
You must be signed in to change notification settings - Fork 0
feat(ui): remove mailing lists and improve project card layout #90
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
- Remove mailing lists from project dashboard, navigation, and home page - Improve responsive grid layout with better breakpoints (sm:grid-cols-2, lg:grid-cols-4) - Enhance project card component with flex layout for consistent heights - Add server-side metrics fetching for meetings and committees count - Temporarily comment out settings menu item until implementation LFXV2-515 Signed-off-by: Asitha de Silva <asithade@gmail.com>
WalkthroughRemoved Mailing Lists references from UI and tests; hid desktop Settings menu item. Updated home grid density and project card layout. Removed Mailing Lists card from project dashboard. Server now enriches projects with meetings_count and committees_count via MeetingService and CommitteeService in getProjects and searchProjects. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant PC as ProjectController
participant PS as ProjectService
participant MS as MeetingService
participant CS as CommitteeService
C->>PC: GET /projects
PC->>PS: fetch projects
PS-->>PC: projects[]
loop for each project
par Parallel counts
PC->>MS: getMeetings(tag=project_uid:<uid>)
PC->>CS: getCommittees(tag=project_uid:<uid>)
and
end
MS-->>PC: meetings[]
CS-->>PC: committees[]
Note right of PC: Set meetings_count and committees_count (fallback to [])
end
PC-->>C: projects[] with counts
%% Similar flow for search
C->>PC: GET /projects/search?q=...
PC->>PS: search projects
PS-->>PC: projects[]
loop enrich each
par counts
PC->>MS: getMeetings(tag=project_uid:<uid>)
PC->>CS: getCommittees(tag=project_uid:<uid>)
end
MS-->>PC: meetings[]
CS-->>PC: committees[]
end
PC-->>C: results[] with counts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes mailing lists functionality from the UI and improves project card layout responsiveness. The changes streamline the project dashboard by removing the mailing lists card section and navigation item while enhancing the grid layout and card presentation.
- Removes mailing lists functionality from project dashboard, navigation, and home page metrics
- Improves responsive grid layout with better breakpoints and consistent card heights
- Adds server-side metrics fetching for meetings and committees count
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| project.controller.ts | Adds server-side metrics fetching for meetings and committees |
| project-card.component.html | Improves flex layout and card height consistency |
| project.component.html | Removes mailing lists card and adjusts grid from 3 to 2 columns |
| home.component.ts | Removes mailing list metrics from project cards |
| home.component.html | Updates grid responsive breakpoints and adds card height class |
| project-layout.component.ts | Removes mailing lists navigation menu item |
| project-layout.component.html | Comments out settings menu temporarily |
| project-dashboard.spec.ts | Updates test expectations for removed menu items |
| homepage.spec.ts | Removes mailing lists metric test assertion |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
✅ E2E Tests PassedBrowser: chromium All E2E tests passed successfully. Test Configuration
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (6)
apps/lfx-one/src/app/layouts/project-layout/project-layout.component.html (1)
71-83: Hide Settings everywhere or gate it; also fix routerLink binding.
- Inconsistent UX: desktop Settings is hidden but mobile link (Lines 59–69) remains. Hide it too until implemented, or guard both behind a feature flag.
- Prefer property binding for routerLink over string interpolation.
Apply:
- <div class="flex md:hidden"> - <a - routerLink="/project/{{ projectSlug() }}/settings" + <!-- TODO: Restore Settings when implemented --> + <!-- <div class="flex md:hidden"> + <a + [routerLink]="['/project', projectSlug(), 'settings']" class="pill" routerLinkActive="bg-blue-50 text-blue-600 border-0 block md:hidden" data-testid="mobile-menu-item" [routerLinkActiveOptions]="{ exact: true }"> <i class="fa-light fa-gear"></i> <span>Settings</span> - </a> - </div> + </a> + </div> -->And pre‑fix the commented desktop block for when it’s re‑enabled:
- routerLink="/project/{{ projectSlug() }}/settings" + [routerLink]="['/project', projectSlug(), 'settings']"Also applies to: 59-69, 61-61, 74-74
apps/lfx-one/e2e/project-dashboard.spec.ts (2)
84-86: Replace commented assertions with explicit “not visible” checks.Keep intent executable and prevent regressions by asserting absence.
- // await expect(page.getByTestId('menu-item').filter({ hasText: 'Mailing Lists' })).toBeVisible(); - // await expect(page.getByTestId('menu-item').filter({ hasText: 'Settings' })).toBeVisible(); + await expect(page.getByTestId('menu-item').filter({ hasText: 'Mailing Lists' })).not.toBeVisible(); + await expect(page.getByTestId('menu-item').filter({ hasText: 'Settings' })).not.toBeVisible();
426-430: Likewise here: assert they’re not present instead of commenting.- // await expect(page.getByTestId('menu-item').filter({ hasText: 'Mailing Lists' })).toBeVisible(); + await expect(page.getByTestId('menu-item').filter({ hasText: 'Mailing Lists' })).not.toBeVisible(); - // await expect(page.getByTestId('menu-item').filter({ hasText: 'Settings' })).toBeVisible(); + await expect(page.getByTestId('menu-item').filter({ hasText: 'Settings' })).not.toBeVisible();apps/lfx-one/src/server/controllers/project.controller.ts (2)
84-92: Apply the same parallelization/logging for search results.- await Promise.all( - results.map(async (project) => { - project.meetings_count = (await this.meetingService.getMeetings(req, { tags: `project_uid:${project.uid}` }).catch(() => [])).length; - project.committees_count = (await this.committeeService.getCommittees(req, { tags: `project_uid:${project.uid}` }).catch(() => [])).length; - }) - ); + await Promise.all( + results.map(async (project) => { + const [meetingsRes, committeesRes] = await Promise.allSettled([ + this.meetingService.getMeetings(req, { tags: `project_uid:${project.uid}` }), + this.committeeService.getCommittees(req, { tags: `project_uid:${project.uid}` }), + ]); + project.meetings_count = meetingsRes.status === 'fulfilled' ? meetingsRes.value.length : 0; + project.committees_count = committeesRes.status === 'fulfilled' ? committeesRes.value.length : 0; + if (meetingsRes.status === 'rejected' || committeesRes.status === 'rejected') { + Logger.warn(req, 'augment_project_metrics_failed', { project_uid: project.uid }); + } + }) + );
140-151: Enrich GET /projects/:slug response with meetings_count and committees_count (or document the difference).getProjects and searchProjects already populate these metrics; add the same augmentation to getProjectBySlug for both the UUID and slug branches in apps/lfx-one/src/server/controllers/project.controller.ts (after fetching the project) — e.g. use Promise.allSettled to call meetingService.getMeetings and committeeService.getCommittees and set project.meetings_count / project.committees_count. packages/shared/src/interfaces/project.interface.ts already declares these fields, so no typing changes are required.
apps/lfx-one/src/app/shared/components/project-card/project-card.component.html (1)
8-11: Solid restructure; add a safe default for unknown badge severities.Layout/testids look good. Provide a neutral style when severity is missing/mistyped.
- <span + <span class="inline-flex items-center px-2.5 py-0.5 rounded-full text-sm font-semibold" [ngClass]="{ 'bg-emerald-500 text-white': metric.badge.severity === 'success', 'bg-blue-500 text-white': metric.badge.severity === 'info', 'bg-amber-500 text-white': metric.badge.severity === 'warning', 'bg-red-500 text-white': metric.badge.severity === 'danger', }" + [class.bg-gray-100]="!['success','info','warning','danger'].includes(metric.badge.severity)" + [class.text-gray-900]="!['success','info','warning','danger'].includes(metric.badge.severity)" data-testid="metric-badge" [attr.data-badge-severity]="metric.badge.severity"> {{ metric.badge.label }} </span>Also applies to: 19-27, 29-57
📜 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 (9)
apps/lfx-one/e2e/homepage.spec.ts(0 hunks)apps/lfx-one/e2e/project-dashboard.spec.ts(2 hunks)apps/lfx-one/src/app/layouts/project-layout/project-layout.component.html(2 hunks)apps/lfx-one/src/app/layouts/project-layout/project-layout.component.ts(0 hunks)apps/lfx-one/src/app/modules/pages/home/home.component.html(2 hunks)apps/lfx-one/src/app/modules/pages/home/home.component.ts(0 hunks)apps/lfx-one/src/app/modules/project/dashboard/project-dashboard/project.component.html(1 hunks)apps/lfx-one/src/app/shared/components/project-card/project-card.component.html(1 hunks)apps/lfx-one/src/server/controllers/project.controller.ts(3 hunks)
💤 Files with no reviewable changes (3)
- apps/lfx-one/src/app/modules/pages/home/home.component.ts
- apps/lfx-one/src/app/layouts/project-layout/project-layout.component.ts
- apps/lfx-one/e2e/homepage.spec.ts
🧰 Additional context used
📓 Path-based instructions (5)
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/layouts/project-layout/project-layout.component.htmlapps/lfx-one/src/app/modules/pages/home/home.component.htmlapps/lfx-one/src/app/modules/project/dashboard/project-dashboard/project.component.htmlapps/lfx-one/src/app/shared/components/project-card/project-card.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/layouts/project-layout/project-layout.component.htmlapps/lfx-one/src/app/modules/pages/home/home.component.htmlapps/lfx-one/src/app/modules/project/dashboard/project-dashboard/project.component.htmlapps/lfx-one/e2e/project-dashboard.spec.tsapps/lfx-one/src/server/controllers/project.controller.tsapps/lfx-one/src/app/shared/components/project-card/project-card.component.html
**/*.{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/e2e/project-dashboard.spec.tsapps/lfx-one/src/server/controllers/project.controller.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use .spec.ts for content-based tests
Files:
apps/lfx-one/e2e/project-dashboard.spec.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not nest ternary expressions
Files:
apps/lfx-one/e2e/project-dashboard.spec.tsapps/lfx-one/src/server/controllers/project.controller.ts
⏰ 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: E2E Tests / Playwright E2E Tests
🔇 Additional comments (3)
apps/lfx-one/src/app/modules/project/dashboard/project-dashboard/project.component.html (1)
159-159: LGTM: grid change aligns with 2-card row.Reducing lg breakpoint to 2 columns matches the removal of the third (Mailing Lists) card and keeps heights consistent.
apps/lfx-one/src/app/modules/pages/home/home.component.html (2)
43-43: LGTM: responsive grid density bump.sm:2 and lg:4 improve utilization without affecting tests (data-testid preserved).
58-59: LGTM: card height consistency via h-full.Aligns with the project-card layout changes for uniform rows.
🚀 Deployment StatusYour branch has been deployed to: https://ui-pr-90.dev.v2.cluster.linuxfound.info Deployment Details:
The deployment will be automatically removed when this PR is closed. |
🧹 Deployment RemovedThe deployment for PR #90 has been removed. |
Summary
Remove mailing lists functionality from the UI and improve project card responsive layout.
JIRA Ticket
LFXV2-515
Changes Made
sm:grid-cols-2breakpoint for better mobile/tablet experience, updated from 3 to 4 columns on large screensTechnical Details
Files Modified:
apps/lfx-one/src/app/layouts/project-layout/project-layout.component.html- Commented out settings menuapps/lfx-one/src/app/layouts/project-layout/project-layout.component.ts- Removed mailing lists navigation itemapps/lfx-one/src/app/modules/pages/home/home.component.html- Improved grid responsive layout and card heightsapps/lfx-one/src/app/modules/pages/home/home.component.ts- Removed mailing list metricsapps/lfx-one/src/app/modules/project/dashboard/project-dashboard/project.component.html- Removed mailing lists card, adjusted gridapps/lfx-one/src/app/shared/components/project-card/project-card.component.html- Enhanced layout with proper flex containersapps/lfx-one/src/server/controllers/project.controller.ts- Added metrics fetching for meetings and committeesTesting
Acceptance Criteria
Generated with Claude Code