[Feature] Projects management | View projects in grid and list view #3594
Conversation
|
|
WalkthroughThe changes update several parts of the codebase to enhance project management functionality. Import paths for date and export components now reference a shared directory. New constants and interface properties have been added while the project interface is expanded with optional fields, additional properties, enums, and a repository interface. API and hook functions are enhanced to support optional query parameters and relations. Additionally, the sidebar and navigation components are updated to include project views. New components for grid and list displays of projects, along with localization updates across multiple languages, enable seamless toggling between views with persisted user preferences. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 ESLint
apps/web/app/[locale]/projects/components/data-table.tsxOops! Something went wrong! :( ESLint: 8.46.0 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/apps/web/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. apps/web/app/[locale]/projects/components/filters-card-modal.tsxOops! Something went wrong! :( ESLint: 8.46.0 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/apps/web/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. apps/web/app/[locale]/projects/components/multi-select-with-search.tsxOops! Something went wrong! :( ESLint: 8.46.0 ESLint couldn't find the config "next/core-web-vitals" to extend from. Please check that the name of the config is correct. The config "next/core-web-vitals" was referenced from the config file in "/apps/web/.eslintrc.json". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team.
✨ Finishing Touches
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 (
|
CREDO23
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (2)
apps/web/components/nav-projects.tsx (1)
85-90: Implement functionality for the "More" button.The "More" button appears to be a placeholder without any functionality. Consider either implementing its intended purpose or removing it if not needed.
apps/web/components/nav-main.tsx (1)
97-103: Improve accessibility for collapsed state.When the sidebar is collapsed, icons are shown without text labels which could impact accessibility. Consider adding aria-labels for screen readers.
{state === 'collapsed' ? ( <SidebarTriggerButton className="!p-0 !bg-inherit !text-inherit"> - <item.icon /> + <item.icon aria-label={item.title} /> </SidebarTriggerButton> ) : ( <item.icon /> )}
🧹 Nitpick comments (9)
apps/web/components/shared/export-mode-select.tsx (1)
5-8: Update JSDoc comments to include the newclassNameprop.The JSDoc comments should be updated to document the new optional
classNameprop for better maintainability.* @param {IProps} props - The component props. * @param {(mode: TExportMode) => void} props.onChange - Function to handle changes in the selected export mode. + * @param {string} [props.className] - Optional CSS classes to apply to the select trigger.apps/web/app/services/client/api/organization-projects.ts (1)
33-37: Consider making relations configurable.The relations array is hardcoded, which might not be optimal for all use cases. Consider making it a parameter with these values as defaults.
-export function getOrganizationProjectsAPI() { +export function getOrganizationProjectsAPI( + customRelations?: string[] +) { // ... - const relations = ['members', 'teams', 'members.employee', 'members.employee.user']; + const relations = customRelations || ['members', 'teams', 'members.employee', 'members.employee.user']; // ...apps/web/components/shared/avatar-stack.tsx (1)
4-7: Make avatar dimensions configurable.The component uses fixed dimensions (8x8). Consider adding size prop for better reusability.
interface AvatarStackProps { avatars: { imageUrl?: string; name: string }[]; maxVisible?: number; + size?: number; // Size in pixels }apps/web/app/interfaces/IOrganizationTeam.ts (1)
104-104: Document the isManager property.Add JSDoc comment explaining the purpose and implications of the isManager flag.
+ /** Indicates whether the member has managerial responsibilities in the team */ isManager: boolean;apps/web/components/nav-projects.tsx (1)
93-104: Enhance create project button validation.The create project button's disabled state only checks for email verification. Consider adding additional validation checks such as user permissions or team role.
apps/web/app/[locale]/project/[id]/components/page-component.tsx (1)
134-147: Refactor duplicate data mapping logic.The same data mapping logic is duplicated between list and grid views. Consider extracting it into a shared function.
+const mapProjectData = (el: IProject) => ({ + project: { + name: el.name, + imageUrl: el.imageUrl, + color: el.color + }, + status: el.status, + startDate: el.startDate, + endDate: el.endDate, + members: el.members, + managers: el.members, + teams: el.teams +}); -data={currentItems.map((el) => ({ - project: { - name: el.name, - imageUrl: el.imageUrl, - color: el.color - }, - status: el.status, - startDate: el.startDate, - endDate: el.endDate, - members: el.members, - managers: el.members, - teams: el.teams -}))} +data={currentItems.map(mapProjectData)}Also applies to: 165-178
apps/web/components/nav-main.tsx (1)
173-195: Simplify nested conditional rendering.The nested conditional rendering with multiple className concatenations is complex and hard to maintain. Consider extracting the logic into separate components or helper functions.
apps/web/app/[locale]/project/[id]/components/data-table.tsx (2)
56-67: Consider memoizing the status color map.The
statusColorsMapis already memoized, but the component's props and state could benefit from memoization to prevent unnecessary re-renders.-export function DataTableProject(props: { data: ProjectTableDataType[] }) { +export function DataTableProject({ data }: { data: ProjectTableDataType[] }) { const [sorting, setSorting] = React.useState<SortingState>([]); const [columnFilters, setColumnFilters] = React.useState<ColumnFiltersState>([]); const [columnVisibility, setColumnVisibility] = React.useState<VisibilityState>({}); const [rowSelection, setRowSelection] = React.useState({}); + const memoizedData = React.useMemo(() => data, [data]); const t = useTranslations(); const { taskStatus } = useTaskStatus();
69-221: Consider extracting column definitions.The column definitions are quite lengthy. Consider extracting them into a separate file or constant for better maintainability.
+// columns.ts +export const getProjectColumns = (t: any, statusColorsMap: Map<string, string>): ColumnDef<ProjectTableDataType>[] => [ + // ... existing column definitions +]; +// data-table.tsx +import { getProjectColumns } from './columns'; + export function DataTableProject({ data }: { data: ProjectTableDataType[] }) { - const columns: ColumnDef<ProjectTableDataType>[] = [ ... ]; + const columns = React.useMemo( + () => getProjectColumns(t, statusColorsMap), + [t, statusColorsMap] + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
apps/web/app/[locale]/project/[id]/components/data-table.tsx(1 hunks)apps/web/app/[locale]/project/[id]/components/grid-item.tsx(1 hunks)apps/web/app/[locale]/project/[id]/components/page-component.tsx(1 hunks)apps/web/app/[locale]/project/[id]/page.tsx(1 hunks)apps/web/app/[locale]/reports/weekly-limit/page.tsx(1 hunks)apps/web/app/constants.ts(1 hunks)apps/web/app/interfaces/IOrganizationTeam.ts(1 hunks)apps/web/app/interfaces/IProject.ts(3 hunks)apps/web/app/services/client/api/organization-projects.ts(2 hunks)apps/web/components/app-sidebar.tsx(4 hunks)apps/web/components/nav-main.tsx(3 hunks)apps/web/components/nav-projects.tsx(1 hunks)apps/web/components/shared/avatar-stack.tsx(1 hunks)apps/web/components/shared/export-mode-select.tsx(3 hunks)apps/web/locales/ar.json(2 hunks)apps/web/locales/bg.json(2 hunks)apps/web/locales/de.json(2 hunks)apps/web/locales/en.json(2 hunks)apps/web/locales/es.json(2 hunks)apps/web/locales/fr.json(2 hunks)apps/web/locales/he.json(2 hunks)apps/web/locales/it.json(2 hunks)apps/web/locales/nl.json(2 hunks)apps/web/locales/pl.json(2 hunks)apps/web/locales/pt.json(2 hunks)apps/web/locales/ru.json(2 hunks)apps/web/locales/zh.json(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/web/app/[locale]/project/[id]/page.tsx
- apps/web/app/[locale]/reports/weekly-limit/page.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: deploy
🔇 Additional comments (28)
apps/web/components/shared/export-mode-select.tsx (2)
22-22: LGTM!Clean props destructuring that follows React best practices.
36-41: LGTM! Width adjustment noted.The styling implementation is clean and follows best practices:
- Proper use of the
cnutility for class merging- Good dark mode support
- Accessible focus states
Note: The width has been reduced from
w-36tow-32. Please ensure this change doesn't affect the layout in existing implementations.apps/web/app/services/client/api/organization-projects.ts (1)
28-31: Add error handling for missing organization or tenant IDs.The function assumes organizationId and tenantId will be available. Consider adding validation to handle cases where these cookies are missing.
if (!organizationId || !tenantId) { throw new Error('Missing required organization or tenant ID'); }apps/web/components/nav-projects.tsx (1)
68-80: Add handlers for dropdown menu actions.The dropdown menu items for viewing, sharing, and deleting projects are not interactive. Consider implementing the necessary handlers to make these actions functional.
Would you like me to help implement the handlers for these actions?
apps/web/app/[locale]/project/[id]/components/data-table.tsx (1)
31-43: LGTM! Well-structured type definition.The
ProjectTableDataTypeinterface is well-defined and properly leverages the existingIProjectinterface.apps/web/app/constants.ts (1)
300-300: LGTM! Consistent naming convention.The new constant follows the existing naming pattern for local storage keys.
apps/web/locales/zh.json (1)
124-127: Verify translations across all language files.Ensure that these new translations are added consistently across all language files (ar.json, bg.json, de.json, etc.) to maintain a complete localization set.
Also applies to: 381-391
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Check if all language files have the new translation keys # List of required keys keys=( "common.START_DATE" "common.END_DATE" "common.MEMBERS" "common.MANAGERS" "pages.projects.projectTitle.PLURAL" "pages.projects.projectTitle.SINGULAR" "pages.projects.CREATE_NEW_PROJECT" "pages.projects.views.LIST_VIEW" "pages.projects.views.GRID_VIEW" ) # Find all language files files=$(fd -e json . apps/web/locales) # Check each key in each file for file in $files; do echo "Checking $file..." for key in "${keys[@]}"; do if ! jq ".$key" "$file" > /dev/null 2>&1; then echo "Missing key: $key" fi done doneLength of output: 11677
Translation consistency verified across all language locales.
The shell script output confirms that all required translation keys (including those added in zh.json) are present across all language files in apps/web/locales (ar.json, bg.json, de.json, en.json, es.json, fr.json, he.json, it.json, nl.json, pl.json, pt.json, ru.json, zh.json).
apps/web/locales/he.json (2)
29-32: New Localization Keys in "common" Section
The keys "START_DATE", "END_DATE", "MEMBERS", and "MANAGERS" have been correctly added with their Hebrew translations. They seamlessly expand the localization support for project management features.
384-394: New "projects" Section Added
The addition of the "projects" section—with nested keys for "projectTitle" (both "PLURAL" and "SINGULAR"), "CREATE_NEW_PROJECT", and a "views" sub-object containing "LIST_VIEW" and "GRID_VIEW"—is consistent with similar localization updates in other languages. This structure will support the grid and list view features as targeted in the PR.apps/web/locales/ar.json (2)
21-24: New Localization Keys in "common" Section
The Arabic localization now includes "START_DATE", "END_DATE", "MEMBERS", and "MANAGERS" with appropriate translations ("تاريخ البدء", "تاريخ الانتهاء", "الأعضاء", "المدراء"). These keys are essential for the upcoming project management functionalities.
384-394: New "projects" Section Added
The new "projects" section is introduced with all necessary nested keys: "projectTitle" (with both plural and singular forms), "CREATE_NEW_PROJECT", and a "views" object containing both "LIST_VIEW" and "GRID_VIEW". The Arabic translations are clear and align with the project's objectives in supporting both grid and list views.apps/web/locales/en.json (2)
330-333: New Localization Keys in Common Section:
The new keys "START_DATE", "END_DATE", "MEMBERS", and "MANAGERS" have been added to the "common" section. They follow the project–related terminology and appear consistent with existing keys.
366-376: New Projects Section with Nested Localization:
A new "projects" section is introduced under the "pages" area. It includes a nested "projectTitle" object for both plural and singular forms, a "CREATE_NEW_PROJECT" key, and a "views" object with "LIST_VIEW" and "GRID_VIEW". Please double-check that the wording matches the intended UI text and aligns stylistically with the rest of the locale file.apps/web/locales/nl.json (1)
28-31: Dutch Localization – Added Keys:
The keys "START_DATE", "END_DATE", "MEMBERS", and "MANAGERS" have been added with the translations "Startdatum", "Einddatum", "Leden", and "Managers" respectively. These entries appear correct and consistent with the application’s context.apps/web/locales/bg.json (1)
26-29: Bulgarian Localization – New Keys Added:
New keys "START_DATE", "END_DATE", "MEMBERS", and "MANAGERS" have been incorporated with translations "Начална дата", "Крайна дата", "Членове", and "Мениджъри". The translations seem appropriate; please verify these match your product’s terminology standards.apps/web/locales/pl.json (2)
27-30: New Localization Keys Added in "common" Section (Polish)
The new keys "START_DATE", "END_DATE", "MEMBERS", and "MANAGERS" have been added with clear and appropriate Polish translations. They integrate well with the existing keys under the "common" section.
425-435: New "projects" Section for Project Management (Polish)
The added "projects" section correctly provides a nested structure with "projectTitle" (both singular and plural forms), "CREATE_NEW_PROJECT", and a "views" sub-object for "LIST_VIEW" and "GRID_VIEW". The translations are consistent and align with the PR objectives for project management views.apps/web/locales/it.json (2)
30-33: New Localization Keys Added in "common" Section (Italian)
The keys "START_DATE", "END_DATE", "MEMBERS", and "MANAGERS" have been introduced with correct Italian translations ("Data di inizio", "Data di fine", "Membri", "Manager"). They maintain a consistent style with the rest of the file.
391-400: New "projects" Section for Project Management (Italian)
The "projects" section is added with a well-structured object that includes "projectTitle" (with "PLURAL" and "SINGULAR"), "CREATE_NEW_PROJECT", and a "views" sub-object containing "LIST_VIEW" ("Vista elenco") and "GRID_VIEW" ("Vista griglia"). These entries meet the PR objectives and are consistent with the translations in other locale files.apps/web/locales/ru.json (2)
29-32: New Localization Keys Added in "common" Section (Russian)
The additions "START_DATE": "Дата начала", "END_DATE": "Дата окончания", "MEMBERS": "Участники", and "MANAGERS": "Менеджеры" are clear and convey the intended meanings accurately. These keys harmonize with the already existing locale entries.
422-432: New "projects" Section for Project Management (Russian)
The new "projects" section is implemented with a nested structure including "projectTitle" (with both "PLURAL" and "SINGULAR"), "CREATE_NEW_PROJECT" ("Создать новый проект"), and a "views" object providing "LIST_VIEW" ("Список") and "GRID_VIEW" ("Сетка"). The translations are correct and align well with the intended functionality described in the PR objectives.apps/web/locales/pt.json (2)
29-32: New Keys in "common" Section:
The additions of "START_DATE", "END_DATE", "MEMBERS", and "MANAGERS" have been introduced with the correct Portuguese translations ("Data de início", "Data de término", "Membros", "Gerentes"). These keys clearly support the new project management features.
426-436: New "projects" Section Added:
A new "projects" section has been created under the localization. It includes a "projectTitle" object with singular and plural keys, "CREATE_NEW_PROJECT", and a nested "views" object with keys for "LIST_VIEW" and "GRID_VIEW". The translations ("Projetos", "Projeto", "Criar novo projeto", "Vista de lista", "Vista em grade") look appropriate. Ensure that these keys are consistently updated in all related locales.apps/web/locales/es.json (1)
444-454: New "projects" Section for Spanish Locale:
The newly added section under "projects" provides project management terminology with keys for "projectTitle" (with "PLURAL" as "Proyectos" and "SINGULAR" as "Proyecto"), "CREATE_NEW_PROJECT", and a nested "views" object with "LIST_VIEW" ("Vista de lista") and "GRID_VIEW" ("Vista de cuadrícula"). These modifications will help support the grid and list view functionality. Verify that the translations align with the overall tone of the application.apps/web/locales/de.json (2)
29-32: New Keys in "common" Section:
The German localization now includes "START_DATE" ("Startdatum"), "END_DATE" ("Enddatum"), "MEMBERS" ("Mitglieder"), and "MANAGERS" ("Manager"). These translations appear accurate and support the features for managing projects.
425-435: New "projects" Section Added:
A new block for project management terms has been added under the "projects" key. It contains "projectTitle" (with "PLURAL" as "Projekte" and "SINGULAR" as "Projekt"), "CREATE_NEW_PROJECT" ("Neues Projekt erstellen"), and a "views" object for toggling between "LIST_VIEW" ("Listenansicht") and "GRID_VIEW" ("Rasteransicht"). This addition aligns with the new grid and list view feature.apps/web/locales/fr.json (2)
30-33: New Common Localization Keys for Project Management
The keys "START_DATE", "END_DATE", "MEMBERS", and "MANAGERS" have been added to the "common" section. Their French translations ("Date de début", "Date de fin", "Membres", "Gestionnaires") are clear and appropriate. Please verify that these keys are consistent with similar changes in other locale files.
422-431: Addition of the Projects Section for View Toggles
A new "projects" section has been introduced with a "projectTitle" object (defining both "PLURAL" as "Projets" and "SINGULAR" as "Projet"), a key for "CREATE_NEW_PROJECT", and a nested "views" object containing "LIST_VIEW" ("Vue liste") and "GRID_VIEW" ("Vue grille"). These entries align well with the new grid and list view functionality detailed in the PR objectives. Ensure that the translations are in line with the UI texts being used across the app.
apps/web/app/[locale]/project/[id]/components/page-component.tsx
Outdated
Show resolved
Hide resolved
apps/web/app/[locale]/project/[id]/components/page-component.tsx
Outdated
Show resolved
Hide resolved
CREDO23
force-pushed
the
3311-feature-projects-management--view-projects-in-grid-and-list-view
branch
from
eaf4e1b to
12e806b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
apps/web/app/interfaces/IProject.ts (1)
25-67:⚠️ Potential issueCore properties should be required for type safety.
Making essential properties like
name,id,organizationId, andtenantIdoptional could lead to runtime issues.Apply this diff to make core properties required:
export interface IProject { - name?: string; + name: string; - id?: string; + id: string; - organizationId?: string; + organizationId: string; - tenantId?: string; + tenantId: string; // ... other properties can remain optional
🧹 Nitpick comments (5)
apps/web/locales/ar.json (2)
21-24: New Localization Keys in "common" Section
The addition of "START_DATE", "END_DATE", "MEMBERS", and "MANAGERS" provides essential labels for the new project management features and is consistent with the related updates in other languages.
384-394: Addition of Projects Section for Enhanced Project Management
The new "projects" section, including the "projectTitle" object (with "PLURAL" and "SINGULAR"), "CREATE_NEW_PROJECT", and the nested "views" (with "LIST_VIEW" and "GRID_VIEW"), aligns well with the PR’s objective to support grid and list views for projects.apps/web/locales/de.json (1)
425-435: Addition of Projects Section for Project Management
The new "projects" section with the "projectTitle" object (both "PLURAL" and "SINGULAR"), "CREATE_NEW_PROJECT", and the nested "views" featuring "LIST_VIEW" and "GRID_VIEW" is a well-structured enhancement that supports the grid and list view functionality introduced in the PR.apps/web/app/interfaces/IProject.ts (2)
49-50: Add JSDoc documentation for time-related properties.The purpose and unit of measurement (days, hours, etc.) for
archiveTasksInandcloseTasksInshould be documented.+ /** Number of days after which tasks should be automatically archived */ archiveTasksIn?: number; + /** Number of days after which tasks should be automatically closed */ closeTasksIn?: number;
22-67: Consider breaking down the large interface.The
IProjectinterface has grown significantly and might be violating the Single Responsibility Principle. Consider splitting it into smaller, focused interfaces:
IProjectCorefor essential propertiesIProjectBillingfor billing-related propertiesIProjectTeamfor team-related propertiesIProjectTaskfor task-related propertiesExample refactor:
interface IProjectCore { id: string; name: string; organizationId: string; tenantId: string; description?: string; // ... other core properties } interface IProjectBilling { billing?: ProjectBillingEnum; currency?: string; billable?: boolean; billingFlat?: boolean; budget?: number; } interface IProjectTeam { members?: OT_Member[]; teams?: IOrganizationTeam[]; defaultAssignee?: IEmployee; membersCount?: number; } interface IProjectTask { tasks?: ITeamTask[]; archiveTasksIn?: number; closeTasksIn?: number; status?: TaskStatusEnum; } export interface IProject extends IProjectCore, IProjectBilling, IProjectTeam, IProjectTask { // ... remaining properties }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
apps/web/app/[locale]/project/[id]/components/data-table.tsx(1 hunks)apps/web/app/[locale]/project/[id]/components/grid-item.tsx(1 hunks)apps/web/app/[locale]/project/[id]/components/page-component.tsx(1 hunks)apps/web/app/[locale]/reports/weekly-limit/page.tsx(1 hunks)apps/web/app/constants.ts(1 hunks)apps/web/app/interfaces/IOrganizationTeam.ts(1 hunks)apps/web/app/interfaces/IProject.ts(3 hunks)apps/web/app/services/client/api/organization-projects.ts(2 hunks)apps/web/components/app-sidebar.tsx(4 hunks)apps/web/components/shared/avatar-stack.tsx(1 hunks)apps/web/components/shared/export-mode-select.tsx(3 hunks)apps/web/locales/ar.json(2 hunks)apps/web/locales/bg.json(2 hunks)apps/web/locales/de.json(2 hunks)apps/web/locales/en.json(2 hunks)apps/web/locales/es.json(2 hunks)apps/web/locales/fr.json(2 hunks)apps/web/locales/he.json(2 hunks)apps/web/locales/it.json(2 hunks)apps/web/locales/nl.json(2 hunks)apps/web/locales/pl.json(2 hunks)apps/web/locales/pt.json(2 hunks)apps/web/locales/ru.json(2 hunks)apps/web/locales/zh.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (19)
- apps/web/app/interfaces/IOrganizationTeam.ts
- apps/web/app/services/client/api/organization-projects.ts
- apps/web/app/constants.ts
- apps/web/components/shared/export-mode-select.tsx
- apps/web/app/[locale]/project/[id]/components/grid-item.tsx
- apps/web/locales/es.json
- apps/web/locales/zh.json
- apps/web/locales/nl.json
- apps/web/app/[locale]/reports/weekly-limit/page.tsx
- apps/web/locales/he.json
- apps/web/locales/it.json
- apps/web/locales/pl.json
- apps/web/locales/pt.json
- apps/web/components/shared/avatar-stack.tsx
- apps/web/locales/en.json
- apps/web/locales/fr.json
- apps/web/locales/bg.json
- apps/web/app/[locale]/project/[id]/components/data-table.tsx
- apps/web/app/[locale]/project/[id]/components/page-component.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: deploy
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (7)
apps/web/locales/ru.json (2)
29-32: New Localization Keys in "common" Section
The keys "START_DATE", "END_DATE", "MEMBERS", and "MANAGERS" have been appropriately added with Russian translations, maintaining consistency with other locales and supporting the new projects management UI.
422-432: Addition of Projects Section with Project-Specific Keys
The new "projects" section introduces a "projectTitle" object (with both "PLURAL" and "SINGULAR"), "CREATE_NEW_PROJECT", and a "views" object containing "LIST_VIEW" and "GRID_VIEW". This perfectly supports the dual view feature for projects.apps/web/locales/de.json (1)
29-32: New Localization Keys in "common" Section
The German localization now includes "START_DATE", "END_DATE", "MEMBERS", and "MANAGERS". These additions enhance the clarity for project-related dates and personnel, meeting the requirements for displaying project management information.apps/web/app/interfaces/IProject.ts (2)
1-6: LGTM!The imports are well-organized and all are utilized in the interface definitions.
79-88: Add JSDoc documentation for enums.The new enums lack documentation explaining their purpose and usage.
+/** + * Defines the billing types available for projects + */ export enum ProjectBillingEnum { RATE = 'RATE', FLAT_FEE = 'FLAT_FEE', MILESTONES = 'MILESTONES' } +/** + * Defines the ownership types for projects + */ export enum ProjectOwnerEnum { CLIENT = 'CLIENT', INTERNAL = 'INTERNAL' }apps/web/components/app-sidebar.tsx (2)
46-50: Add error handling for project fetching.The
getOrganizationProjectscall should handle potential errors to prevent the UI from breaking.
238-242: Implement archived projects functionality.The "Archived projects" section is currently a placeholder without functionality. Consider:
- Adding a filter for archived projects
- Implementing the navigation
- Adding a count of archived projects
Let me check if there's an API endpoint for archived projects:
CREDO23
force-pushed
the
3311-feature-projects-management--view-projects-in-grid-and-list-view
branch
from
12e806b to
5a3f28e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
apps/web/components/app-sidebar.tsx (4)
46-50:⚠️ Potential issueAdd error and loading state handling.
The project fetching implementation should handle errors and loading states.
Apply this diff to handle errors and loading states:
-const { getOrganizationProjects, organizationProjects } = useOrganizationProjects(); +const { getOrganizationProjects, organizationProjects, error, loading } = useOrganizationProjects(); useEffect(() => { - getOrganizationProjects(); + getOrganizationProjects().catch((err) => { + console.error('Failed to fetch projects:', err); + }); }, [getOrganizationProjects]);
205-244:⚠️ Potential issueAdd loading and error states for projects section.
The projects section should handle loading and error states gracefully.
Apply this diff to handle loading and error states:
{ title: t('sidebar.PROJECTS'), url: '/projects', icon: FolderKanban, label: 'projects', items: [ + loading ? [{ title: 'Loading...', label: 'loading' }] : + error ? [{ title: 'Failed to load projects', label: 'error' }] : + organizationProjects.length === 0 ? [{ title: 'No projects', label: 'no-projects' }] : [ ...organizationProjects.map((project) => ({ // ... existing project mapping })), { title: 'Archived projects', url: '', label: 'Archived projects' } ] ] }
213-214:⚠️ Potential issueAdd fallback for undefined project name.
The project name should have a fallback value to prevent empty titles.
Apply this diff to add a fallback:
-title: project.name ?? '', +title: project.name ?? 'Untitled Project',
226-232: 🛠️ Refactor suggestion
⚠️ Potential issueAdd error handling and accessibility improvements for project icon.
The project icon needs error handling for failed image loads and accessibility improvements.
Apply this diff to improve the implementation:
<Image - alt={project.name ?? ''} + alt={`${project.name ?? 'Project'} icon`} height={40} width={40} className="w-full h-full" src={project.imageUrl} + onError={(e) => { + e.currentTarget.src = ''; // Clear the broken image + e.currentTarget.style.display = 'none'; + e.currentTarget.parentElement.textContent = project.name?.substring(0, 2) ?? 'UN'; + }} />Also, add an aria-label to the icon container:
<div style={{ backgroundColor: project.color }} className={cn( 'w-8 h-8 border overflow-hidden flex items-center justify-center rounded-full' )} + aria-label={`${project.name ?? 'Project'} icon`} + role="img" >
🧹 Nitpick comments (2)
apps/web/components/nav-main.tsx (1)
173-195: Enhance accessibility and theme consistency.While the layout structure is good, there are a few improvements to consider:
- The Link should wrap the entire content for better accessibility (larger clickable area)
- The icon container border needs dark mode support
- The gap between icon and text should match parent menu items for consistency
Consider this implementation:
-<div className="w-full h-full flex items-center gap-2"> - {subItem.icon && ( - <div - className={cn( - 'w-8 h-8 border rounded-full flex items-center justify-center' - )} - > - {subItem.icon} - </div> - )} - <Link href={subItem.url}> - <span - className={cn( - 'transition-all font-light !text-sm', - state === 'collapsed' - ? 'opacity-0 hidden' - : 'opacity-100' - )} - > - {subItem.title} - </span> - </Link> -</div> +<Link href={subItem.url} className="w-full h-full flex items-center gap-[10px]"> + {subItem.icon && ( + <div + className={cn( + 'w-8 h-8 border dark:border-gray-600 rounded-full flex items-center justify-center' + )} + > + {subItem.icon} + </div> + )} + <span + className={cn( + 'transition-all font-light !text-sm', + state === 'collapsed' + ? 'opacity-0 hidden' + : 'opacity-100' + )} + > + {subItem.title} + </span> +</Link>apps/web/app/interfaces/IProject.ts (1)
25-66: Consider grouping related properties.The interface properties could be better organized by grouping related fields together. Consider reordering properties into logical groups:
- Core properties (id, name, code, description, etc.)
- Dates (startDate, endDate, createdAt, etc.)
- Billing (billing, currency, billable, etc.)
- Status (status, isActive, isArchived)
- Relations (members, teams, tasks, etc.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
apps/web/app/[locale]/project/[id]/components/data-table.tsx(1 hunks)apps/web/app/[locale]/project/[id]/components/grid-item.tsx(1 hunks)apps/web/app/[locale]/project/[id]/components/page-component.tsx(1 hunks)apps/web/app/[locale]/project/[id]/page.tsx(1 hunks)apps/web/app/[locale]/reports/weekly-limit/page.tsx(1 hunks)apps/web/app/constants.ts(1 hunks)apps/web/app/interfaces/IOrganizationTeam.ts(1 hunks)apps/web/app/interfaces/IProject.ts(3 hunks)apps/web/app/services/client/api/organization-projects.ts(2 hunks)apps/web/components/app-sidebar.tsx(4 hunks)apps/web/components/nav-main.tsx(3 hunks)apps/web/components/nav-projects.tsx(1 hunks)apps/web/components/shared/avatar-stack.tsx(1 hunks)apps/web/components/shared/export-mode-select.tsx(3 hunks)apps/web/locales/ar.json(2 hunks)apps/web/locales/bg.json(2 hunks)apps/web/locales/de.json(2 hunks)apps/web/locales/en.json(2 hunks)apps/web/locales/es.json(2 hunks)apps/web/locales/fr.json(2 hunks)apps/web/locales/he.json(2 hunks)apps/web/locales/it.json(2 hunks)apps/web/locales/nl.json(2 hunks)apps/web/locales/pl.json(2 hunks)apps/web/locales/pt.json(2 hunks)apps/web/locales/ru.json(2 hunks)apps/web/locales/zh.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (21)
- apps/web/app/constants.ts
- apps/web/app/interfaces/IOrganizationTeam.ts
- apps/web/app/[locale]/project/[id]/page.tsx
- apps/web/app/services/client/api/organization-projects.ts
- apps/web/app/[locale]/reports/weekly-limit/page.tsx
- apps/web/components/shared/export-mode-select.tsx
- apps/web/app/[locale]/project/[id]/components/data-table.tsx
- apps/web/app/[locale]/project/[id]/components/page-component.tsx
- apps/web/components/nav-projects.tsx
- apps/web/locales/nl.json
- apps/web/components/shared/avatar-stack.tsx
- apps/web/locales/de.json
- apps/web/locales/ru.json
- apps/web/locales/pt.json
- apps/web/app/[locale]/project/[id]/components/grid-item.tsx
- apps/web/locales/bg.json
- apps/web/locales/fr.json
- apps/web/locales/he.json
- apps/web/locales/pl.json
- apps/web/locales/it.json
- apps/web/locales/zh.json
🔇 Additional comments (10)
apps/web/components/nav-main.tsx (1)
21-21: LGTM! Type definitions are well-structured.The addition of the optional
iconproperty usingReactNodetype provides good flexibility for rendering various icon types in sub-menu items.Also applies to: 35-35
apps/web/locales/ar.json (2)
21-24: New Localization Keys for Dates and Roles
The new keys "START_DATE", "END_DATE", "MEMBERS", and "MANAGERS" have been added to the "common" section with clear and contextually appropriate Arabic translations. Please double-check that they match the project's overall terminology.
384-394: New Projects Section Added
A new "projects" section has been introduced with keys for "projectTitle" (PLURAL and SINGULAR), "CREATE_NEW_PROJECT", and the "views" sub-object. The Arabic translations ("المشاريع", "المشروع", "إنشاء مشروع جديد", "عرض القائمة", "عرض الشبكة") are correct and consistent. Verify that this structure mirrors the intended UI design for the project management feature.apps/web/locales/en.json (2)
330-333: New Localization Keys for Dates and Roles in English
The added keys "START_DATE", "END_DATE", "MEMBERS", and "MANAGERS" now provide clear English labels. The terminology aligns well with the project management context.
366-376: Projects Section for Project Management
A new "projects" section has been introduced with a nested "projectTitle" object (defining both PLURAL and SINGULAR forms), a "CREATE_NEW_PROJECT" key, and a "views" sub-object for "LIST_VIEW" and "GRID_VIEW". The phrasing is clear and consistent with other UI elements.apps/web/locales/es.json (2)
249-252: New Localization Keys for Dates and Roles in Spanish
The new keys have been added with the translations "Fecha de inicio", "Fecha de finalización", "Miembros" and "Gerentes". These additions are contextually appropriate and match the style used in other parts of the application.
444-454: Projects Section for Enhanced Project Management
A new "projects" object is present, containing "projectTitle" (with "PLURAL" and "SINGULAR"), "CREATE_NEW_PROJECT", and a "views" object for "LIST_VIEW" and "GRID_VIEW". The Spanish translations ("Proyectos", "Proyecto", "Crear nuevo proyecto", "Vista de lista", "Vista de cuadrícula") are accurate. Ensure consistency with the overall localization strategy across the app.apps/web/app/interfaces/IProject.ts (2)
1-5: LGTM!The imports are well-organized and all are used within the interface definitions.
79-88: Add JSDoc documentation for enums.The new enums lack documentation explaining their purpose and usage.
Apply this diff:
+/** + * Defines the billing types available for projects + */ export enum ProjectBillingEnum { RATE = 'RATE', FLAT_FEE = 'FLAT_FEE', MILESTONES = 'MILESTONES' } +/** + * Defines the ownership types for projects + */ export enum ProjectOwnerEnum { CLIENT = 'CLIENT', INTERNAL = 'INTERNAL' }apps/web/components/app-sidebar.tsx (1)
11-14: LGTM! Well-organized imports.The new imports are properly organized and necessary for implementing the project management functionality.
Also applies to: 29-29, 36-36
|
Yes @evereq , I made it based on figma! So, yes, Accordingly to the figma, we have a dropdow that show all projects in the sidebar! ( Maybe it should be teams accordingly to you ) |
|
@CREDO23 the logic probably was this: However, I do have a question - in the siderbar, we show names of projects that are DIFFERENT to names you show in the table view (e.g. TeamBS in sidebar, but in table you show Project 67 and similar), so I think in the sidebar we still show list of teams, not really projects and it's totally wrong... |
|
@CREDO23 another consideration is that list of projects displayed in Sidebar (or in the table if "Projects" item selected in Sidebar), should be ONLY containing projects that are for selected team in the site header! So if I selected Team A, we can ONLy see projects that belong to that Team A, not all projects. However of course user can select "All" Teams in the site header and in that case we need to display ALL projects from ALL teams! That means we may need to consider some sort of scrolling for the sidebar if amount of projects is very large! Such scroll should be displayed either inside expanded "Projects" menu item in the siderbar or it should be one scroll for whole Sidebar (displayed ONLY if sidebar items not fit on screen). CC: @NdekoCode see above comments, thanks |
@evereq , I used dummy data in the list (table) view ! |
That's the reason projects in the sidebar are different from those in the page |
|
No dependency changes detected. Learn more about Socket for GitHub ↗︎ 👍 No dependency changes detected in pull request |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
apps/web/app/[locale]/projects/components/page-component.tsx (1)
66-68: Add error handling for project fetching.The
getOrganizationProjectscall should handle potential errors to prevent the UI from breaking.apps/web/app/[locale]/projects/components/data-table.tsx (1)
113-120: Add error handling for project image.The Image component should handle loading and error states to prevent UI issues when images fail to load.
apps/web/components/app-sidebar.tsx (3)
222-222: Add fallback for undefined project name.The project name is accessed with optional chaining but no fallback is provided. This could result in an empty title.
235-241: Add error handling for project image.The Image component should handle loading and error states to prevent UI issues when images fail to load.
219-247: Add loading state for projects section.The projects section should show a loading state while fetching data and handle empty states gracefully.
🧹 Nitpick comments (6)
apps/web/components/shared/date-range-select.tsx (1)
46-51: Refactor className string for better maintainability.The className string has some issues:
- Duplicate height class
h-[2.2rem]- Inconsistent spacing
- Long, hard-to-maintain className string
Consider applying this diff to improve the implementation:
className={cn( 'w-[16rem] h-[2.2rem] justify-start text-left font-light', !date && 'text-muted-foreground', - 'overflow-hidden h-[2.2rem] text-clip border border-gray-200 dark:border-gray-700 bg-white dark:bg-dark--theme-light focus:ring-2 focus:ring-transparent', + 'overflow-hidden text-clip border border-gray-200 dark:border-gray-700', + 'bg-white dark:bg-dark--theme-light focus:ring-2 focus:ring-transparent', className )}apps/web/app/services/client/api/organization-projects.ts (1)
24-43: Add type safety for query parameters.While the dynamic query support is good, using
Record<string, string>is too permissive and could lead to runtime errors.Consider creating a typed interface for query parameters:
interface ProjectQueries { status?: string; search?: string; sortBy?: 'name' | 'startDate' | 'endDate'; order?: 'ASC' | 'DESC'; // Add other valid query parameters } export function getOrganizationProjectsAPI({ queries }: { queries?: ProjectQueries } = {}) { // ... rest of the code }apps/web/app/hooks/features/useOrganizationProjects.ts (1)
72-72: Remove debug console.log.Debug logging should not be present in production code.
- console.log(res.data);apps/web/app/[locale]/projects/components/grid-item.tsx (1)
58-58: Consider responsive design for grid items.The fixed width of 24rem might not work well on different screen sizes.
-<div className="w-[24rem] shrink-0 border rounded-xl p-4 gap-4 flex"> +<div className="w-full sm:w-[20rem] md:w-[24rem] shrink-0 border rounded-xl p-4 gap-4 flex">apps/web/app/[locale]/projects/components/page-component.tsx (1)
70-118: Add loading states to improve user experience.The UI should show loading states while fetching data to provide better feedback to users.
Apply this diff to add loading states:
<MainLayout showTimer={isTrackingEnabled} className="!p-0 pb-1 !overflow-hidden w-full" childrenClassName="w-full h-full" mainHeaderSlot={ + loading ? ( + <div className="p-4 animate-pulse"> + <div className="h-8 bg-gray-200 rounded w-1/4 mb-4"></div> + <div className="h-10 bg-gray-200 rounded w-full"></div> + </div> + ) : ( <div className="flex flex-col p-4 dark:bg-dark--theme"> // ... existing code </div> + ) } >apps/web/app/[locale]/projects/components/data-table.tsx (1)
58-61: Consider persisting table state.The table state (sorting, filters, column visibility) could be persisted to improve user experience across sessions.
Apply this diff to persist table state:
+const STORAGE_KEY = 'project_table_state'; + +const getStoredState = () => { + try { + const stored = localStorage.getItem(STORAGE_KEY); + return stored ? JSON.parse(stored) : null; + } catch (error) { + console.error('Failed to get stored table state:', error); + return null; + } +}; + +const saveState = (state: any) => { + try { + localStorage.setItem(STORAGE_KEY, JSON.stringify(state)); + } catch (error) { + console.error('Failed to save table state:', error); + } +}; + -const [sorting, setSorting] = React.useState<SortingState>([]); -const [columnFilters, setColumnFilters] = React.useState<ColumnFiltersState>([]); -const [columnVisibility, setColumnVisibility] = React.useState<VisibilityState>({}); +const stored = getStoredState(); +const [sorting, setSorting] = React.useState<SortingState>(stored?.sorting ?? []); +const [columnFilters, setColumnFilters] = React.useState<ColumnFiltersState>(stored?.filters ?? []); +const [columnVisibility, setColumnVisibility] = React.useState<VisibilityState>(stored?.visibility ?? {}); + +React.useEffect(() => { + saveState({ sorting, filters: columnFilters, visibility: columnVisibility }); +}, [sorting, columnFilters, columnVisibility]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/web/app/[locale]/projects/[id]/page.tsx(1 hunks)apps/web/app/[locale]/projects/components/data-table.tsx(1 hunks)apps/web/app/[locale]/projects/components/grid-item.tsx(1 hunks)apps/web/app/[locale]/projects/components/page-component.tsx(1 hunks)apps/web/app/[locale]/projects/page.tsx(1 hunks)apps/web/app/hooks/features/useOrganizationProjects.ts(3 hunks)apps/web/app/services/client/api/organization-projects.ts(2 hunks)apps/web/components/app-sidebar.tsx(8 hunks)apps/web/components/nav-main.tsx(4 hunks)apps/web/components/shared/date-range-select.tsx(2 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- apps/web/app/[locale]/projects/page.tsx
- package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/components/nav-main.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: deploy
- GitHub Check: deploy
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (6)
apps/web/components/shared/date-range-select.tsx (1)
26-34: LGTM! Well-structured component signature.The addition of the optional
classNameprop follows TypeScript best practices and enhances the component's styling flexibility.apps/web/app/[locale]/projects/components/grid-item.tsx (2)
59-59: Implement checkbox functionality.The checkbox is rendered but lacks onChange handler and checked state management.
Implement selection functionality:
-<Checkbox className=" shrink-0" /> +<Checkbox + className="shrink-0" + checked={isSelected} + onCheckedChange={onSelect} />Please verify if project selection is needed for bulk actions or other features.
83-83: Implement ellipsis menu functionality.The ellipsis icon is rendered but lacks click handler and menu options.
Add menu functionality:
-<Ellipsis size={20} /> +<DropdownMenu> + <DropdownMenuTrigger> + <Ellipsis size={20} className="cursor-pointer" /> + </DropdownMenuTrigger> + <DropdownMenuContent> + {/* Add menu items for edit, delete, etc. */} + </DropdownMenuContent> +</DropdownMenu>Please verify the required menu options for project management.
apps/web/app/[locale]/projects/components/page-component.tsx (1)
26-33: LGTM! Good error handling for localStorage.The implementation properly handles potential localStorage access errors and provides a fallback.
apps/web/app/[locale]/projects/components/data-table.tsx (2)
31-43: LGTM! Well-structured type definitions.The
ProjectTableDataTypeis well-defined and properly references theIProjectinterface.
244-291: LGTM! Good empty state handling.The table component properly handles empty states and provides appropriate feedback to users.
CREDO23
force-pushed
the
3311-feature-projects-management--view-projects-in-grid-and-list-view
branch
from
affa59a to
e64a212
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
apps/web/app/[locale]/projects/components/page-component.tsx (1)
124-128: Implement missing core functionalities.Several key features are marked as TODO and not implemented:
- Search functionality
- Date range filtering
- Export handling
These features are essential for a complete project management experience.
Also applies to: 133-135, 147-150
apps/web/components/app-sidebar.tsx (2)
235-241: Add error handling for project images.The Image component should handle loading and error states to prevent UI issues.
222-222: Improve project name fallback.The current fallback for undefined project names could result in an empty title.
🧹 Nitpick comments (2)
apps/web/app/[locale]/projects/components/page-component.tsx (1)
26-33: Enhance local storage error handling.While error handling is present, consider providing a fallback mechanism when local storage is unavailable.
try { return localStorage.getItem(LAST_SELECTED_PROJECTS_VIEW) as TViewMode; } catch (error) { console.error('Failed to access localStorage:', error); - return null; + return 'LIST'; // Provide a default view mode }apps/web/app/[locale]/projects/components/data-table.tsx (1)
163-164: Improve date formatting for better localization.Consider using the
Intl.DateTimeFormatAPI instead of moment.js for better localization support and reduced bundle size.-{row.original?.startDate && moment(row.original?.startDate).format('MMM. DD YYYY')} +{row.original?.startDate && new Intl.DateTimeFormat(undefined, { + year: 'numeric', + month: 'short', + day: '2-digit' +}).format(new Date(row.original.startDate))}Also applies to: 178-179
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/web/app/[locale]/projects/[id]/page.tsx(1 hunks)apps/web/app/[locale]/projects/components/data-table.tsx(1 hunks)apps/web/app/[locale]/projects/components/grid-item.tsx(1 hunks)apps/web/app/[locale]/projects/components/page-component.tsx(1 hunks)apps/web/app/[locale]/projects/page.tsx(1 hunks)apps/web/app/hooks/features/useOrganizationProjects.ts(3 hunks)apps/web/app/services/client/api/organization-projects.ts(2 hunks)apps/web/components/app-sidebar.tsx(8 hunks)apps/web/components/nav-main.tsx(4 hunks)apps/web/components/shared/date-range-select.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/web/app/[locale]/projects/page.tsx
- apps/web/app/[locale]/projects/[id]/page.tsx
- apps/web/app/services/client/api/organization-projects.ts
- apps/web/app/hooks/features/useOrganizationProjects.ts
- apps/web/components/shared/date-range-select.tsx
- apps/web/app/[locale]/projects/components/grid-item.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: deploy
🔇 Additional comments (5)
apps/web/components/nav-main.tsx (2)
70-103: Good job extracting theItemContentcomponent!The extraction of common rendering logic into a separate component follows the DRY principle and improves code maintainability.
132-140: Well-structured conditional rendering with Link.The conditional wrapping of
ItemContentwith Link based on theselectableproperty provides good flexibility and control over navigation behavior.apps/web/app/[locale]/projects/components/page-component.tsx (1)
84-106: Well-implemented view mode toggle with persistence.The view mode toggle implementation is clean and includes proper state persistence in local storage.
apps/web/app/[locale]/projects/components/data-table.tsx (1)
65-67: Efficient status color mapping implementation.Good use of
useMemoto cache the status colors map and prevent unnecessary recalculations.apps/web/components/app-sidebar.tsx (1)
51-54: Well-implemented project filtering with memoization.Good use of
useMemoto efficiently filter projects based on the current path and active team.
CREDO23
force-pushed
the
3311-feature-projects-management--view-projects-in-grid-and-list-view
branch
from
e64a212 to
ced9d17
Compare
|
@CREDO23 hm, you suppose to tell somewhere in PR that you used dummy data 😢 . Why we use Dummy data if we have APIs for Projects and can just load real data instead? Can you do it please? |
CREDO23
force-pushed
the
3311-feature-projects-management--view-projects-in-grid-and-list-view
branch
from
ced9d17 to
4f5c2bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
apps/web/components/app-sidebar.tsx (2)
219-247:⚠️ Potential issueAdd error handling and loading states for projects section.
The projects section should handle loading states and errors gracefully.
items: [ + loading ? [{ title: 'Loading...', label: 'loading' }] : + error ? [{ title: 'Failed to load projects', label: 'error' }] : + !projects ? [{ title: 'No projects', label: 'no-projects' }] : ...(projects ? projects.map((project) => {
232-242:⚠️ Potential issueAdd error handling for project image and improve fallback.
The Image component should handle loading and error states. Also, the project name fallback could be improved.
-{!project.imageUrl ? ( - project.name?.substring(0, 2) -) : ( - <Image - alt={project.name ?? ''} - height={40} - width={40} - className="w-full h-full" - src={project.imageUrl} - /> -)} +{!project.imageUrl ? ( + <span className="text-sm font-medium"> + {project.name?.substring(0, 2).toUpperCase() ?? 'UN'} + </span> +) : ( + <Image + alt={project.name ?? 'Project'} + height={40} + width={40} + className="w-full h-full" + src={project.imageUrl} + onError={(e) => { + e.currentTarget.src = ''; + e.currentTarget.style.display = 'none'; + e.currentTarget.parentElement.textContent = + project.name?.substring(0, 2).toUpperCase() ?? 'UN'; + }} + /> +)}
🧹 Nitpick comments (12)
apps/web/app/[locale]/projects/components/grid-item.tsx (5)
12-14: Consider enhancing the interface documentation.Add JSDoc comments to describe the purpose of the component and its props for better maintainability.
+/** + * Props for the GridItem component that displays project information in a grid format + */ interface IGridItemProps { + /** Project data containing details about members, teams, status, and dates */ data: ProjectTableDataType; }
21-53: Optimize memoized computations and improve type safety.The current implementation could benefit from:
- Using a more efficient data structure for status colors
- Simplifying the mapping logic
- Adding type assertions for better type safety
-const statusColorsMap: Map<string | undefined, string | undefined> = useMemo(() => { - return new Map(taskStatus.map((status) => [status.name, status.color])); +const statusColorsMap = useMemo(() => { + return Object.fromEntries(taskStatus.map((status) => [status.name, status.color])) as Record<string, string>; }, [taskStatus]); const members = useMemo( () => - data?.members - ?.filter((el) => !el.isManager) - ?.map((el) => ({ + data?.members?.reduce<Array<{ imageUrl?: string; name?: string }>>((acc, el) => { + if (!el.isManager && el.employee) { + acc.push({ imageUrl: el?.employee?.user?.imageUrl, name: el?.employee?.fullName - })) || [], + }); + } + return acc; + }, []) ?? [], [data?.members] );
57-59: Improve responsiveness and accessibility.The hardcoded width and shrink properties might limit responsiveness on different screen sizes.
-<div className="w-[24rem] shrink-0 border rounded-xl p-4 gap-4 flex"> - <Checkbox className=" shrink-0" /> +<div className="w-full sm:w-[24rem] border rounded-xl p-4 gap-4 flex"> + <Checkbox className="shrink-0" aria-label="Select project" />
104-108: Replace moment.js with modern date handling.Consider using
date-fnsor the nativeIntl.DateTimeFormatfor better performance and bundle size.-import moment from 'moment'; +import { format } from 'date-fns'; -<p>{moment(data?.startDate).format('D.MM.YYYY')}</p> +<p>{data?.startDate ? format(new Date(data.startDate), 'dd.MM.yyyy') : '-'}</p> -<p>{moment(data?.endDate).format('D.MM.YYYY')}</p> +<p>{data?.endDate ? format(new Date(data.endDate), 'dd.MM.yyyy') : '-'}</p>Also applies to: 118-122
130-145: Extract member sections into separate components.The member, team, and manager sections share similar structure and could be extracted into a reusable component.
interface EntitySectionProps { title: string; avatars: Array<{ imageUrl?: string; name?: string }>; maxVisible: number; } function EntitySection({ title, avatars, maxVisible }: EntitySectionProps) { const t = useTranslations(); return ( <div className="w-full flex flex-col gap-2"> <p className="font-medium">{t(title)}</p> <div>{avatars?.length > 0 ? <AvatarStack maxVisible={maxVisible} avatars={avatars} /> : '-'}</div> </div> ); }Then use it in the component:
-<div className="w-full flex flex-col gap-2"> - <p className="font-medium">{t('common.MEMBERS')}</p> - <div>{members?.length > 0 ? <AvatarStack maxVisible={3} avatars={members} /> : '-'}</div> -</div> +<EntitySection title="common.MEMBERS" avatars={members} maxVisible={3} />apps/web/app/services/client/api/organization-projects.ts (3)
31-31: Consider using type declaration instead of type assertion.Type assertions should be avoided when possible as they can mask type errors. Consider declaring the type upfront:
- } as Record<string, string>; + const obj: Record<string, string> = { + 'where[organizationId]': organizationId, + 'where[tenantId]': tenantId + };
33-37: Consider extracting relations to a constant.The relations array should be extracted to a constant outside the function to prevent recreation on each call.
+const PROJECT_RELATIONS = ['members', 'teams', 'members.employee', 'members.employee.user'] as const; + export function getOrganizationProjectsAPI({ queries }: { queries?: Record<string, string> } = {}) { // ... - const relations = ['members', 'teams', 'members.employee', 'members.employee.user']; - - relations.forEach((relation, i) => { + PROJECT_RELATIONS.forEach((relation, i) => { obj[`relations[${i}]`] = relation; });
39-43: Consider using Object.assign for merging queries.The current implementation using forEach could be simplified using Object.assign.
- if (queries) { - Object.entries(queries).forEach(([key, value]) => { - obj[key] = value; - }); - } + if (queries) { + Object.assign(obj, queries); + }apps/web/app/[locale]/projects/components/data-table.tsx (4)
32-44: Consider using TypeScript utility types for better type safety.The type definition could be improved using TypeScript's utility types for better maintainability and type safety.
-export type ProjectTableDataType = { - project: { - name: IProject['name']; - imageUrl: IProject['imageUrl']; - color: IProject['color']; - }; - status: IProject['status']; - startDate: IProject['startDate']; - endDate: IProject['endDate']; - members: IProject['members']; - managers: IProject['members']; - teams: IProject['teams']; -}; +export type ProjectTableDataType = { + project: Pick<IProject, 'name' | 'imageUrl' | 'color'>; +} & Pick<IProject, 'status' | 'startDate' | 'endDate' | 'members' | 'teams'> & { + managers: IProject['members']; +};
246-248: Remove debug console.log statement.The useEffect hook contains a console.log statement that should be removed before production.
-React.useEffect(() => { - console.log(loading); -}, [loading]);
67-69: Optimize status colors map initialization.The status colors map could be initialized outside the component to avoid unnecessary recreations.
-const statusColorsMap: Map<string | undefined, string | undefined> = useMemo(() => { - return new Map(taskStatus.map((status) => [status.name, status.color])); -}, [taskStatus]); +const statusColorsMap = useMemo(() => + taskStatus.reduce((map, status) => { + map.set(status.name, status.color); + return map; + }, new Map<string | undefined, string | undefined>()) +, [taskStatus]);
164-166: Replace moment.js with modern date handling.Consider using modern alternatives like
date-fnsorIntl.DateTimeFormatinstead of the legacy moment.js library.-{row.original?.startDate && moment(row.original?.startDate).format('MMM. DD YYYY')} +{row.original?.startDate && new Intl.DateTimeFormat('en-US', { + month: 'short', + day: '2-digit', + year: 'numeric' +}).format(new Date(row.original.startDate))}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/web/app/[locale]/projects/[id]/page.tsx(1 hunks)apps/web/app/[locale]/projects/components/data-table.tsx(1 hunks)apps/web/app/[locale]/projects/components/grid-item.tsx(1 hunks)apps/web/app/[locale]/projects/components/page-component.tsx(1 hunks)apps/web/app/[locale]/projects/page.tsx(1 hunks)apps/web/app/hooks/features/useOrganizationProjects.ts(3 hunks)apps/web/app/services/client/api/organization-projects.ts(2 hunks)apps/web/components/app-sidebar.tsx(8 hunks)apps/web/components/nav-main.tsx(4 hunks)apps/web/components/shared/date-range-select.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/web/app/[locale]/projects/page.tsx
- apps/web/app/hooks/features/useOrganizationProjects.ts
- apps/web/app/[locale]/projects/[id]/page.tsx
- apps/web/components/shared/date-range-select.tsx
- apps/web/app/[locale]/projects/components/page-component.tsx
- apps/web/components/nav-main.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: deploy
🔇 Additional comments (1)
apps/web/app/[locale]/projects/components/grid-item.tsx (1)
19-19: Add error handling for the useTaskStatus hook.Consider handling potential errors or loading states from the
useTaskStatushook to ensure graceful degradation.-const { taskStatus } = useTaskStatus(); +const { taskStatus, isLoading, error } = useTaskStatus(); + +if (isLoading) { + return <div>Loading...</div>; +} + +if (error) { + console.error('Failed to load task statuses:', error); + return <div>Error loading project information</div>; +}
…t-search-and-filter-project-in-the-list-page [Feature] Projects management | Search and filter projects in the list and grid page
evereq
deleted the
3311-feature-projects-management--view-projects-in-grid-and-list-view
branch
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
apps/web/app/[locale]/projects/components/page-component.tsx (1)
170-174:⚠️ Potential issueImplement missing functionality.
The search, date range, and export functionality are not implemented.
Would you like me to help implement these missing features?
Also applies to: 179-181, 194-197
🧹 Nitpick comments (7)
apps/web/app/services/client/api/organization-projects.ts (1)
24-52: Consider optimizing data fetching for better performance.The function fetches multiple relations (
members,teams,members.employee,members.employee.user) for each project, which could impact performance, especially with a large number of projects.Consider:
- Making relations optional through query parameters to allow selective loading.
- Implementing pagination for related data.
export function getOrganizationProjectsAPI({ queries }: { queries?: Record<string, string> } = {}) { const organizationId = getOrganizationIdCookie(); const tenantId = getTenantIdCookie(); const obj = { 'where[organizationId]': organizationId, 'where[tenantId]': tenantId, 'join[alias]': 'organization_project', 'join[leftJoin][tags]': 'organization_project.tags' } as Record<string, string>; - const relations = ['members', 'teams', 'members.employee', 'members.employee.user']; + const defaultRelations = ['members', 'teams']; + const extendedRelations = ['members.employee', 'members.employee.user']; + const relations = queries?.loadExtendedRelations === 'true' + ? [...defaultRelations, ...extendedRelations] + : defaultRelations; relations.forEach((relation, i) => { obj[`relations[${i}]`] = relation; }); if (queries) { Object.entries(queries).forEach(([key, value]) => { obj[key] = value; }); } const query = qs.stringify(obj); return get<PaginationResponse<IProject>>(`/organization-projects?${query}`, { tenantId }); }apps/web/app/[locale]/projects/components/page-component.tsx (2)
96-114: Optimize project filtering for better performance.The current implementation filters projects on every render, which could impact performance with a large number of projects.
Apply this diff to optimize project filtering:
-const filteredProjects = useMemo( - () => - currentItems - ?.filter((el) => activeTeamProjects.includes(el.id)) - ?.map((el) => ({ - project: { - name: el.name, - imageUrl: el.imageUrl, - color: el.color - }, - status: el.status, - startDate: el.startDate, - endDate: el.endDate, - members: el.members, - managers: el.members, - teams: el.teams - })), - [currentItems, activeTeamProjects] -); +const filteredProjects = useMemo(() => { + if (!currentItems?.length || !activeTeamProjects?.length) { + return []; + } + + const projectMap = new Set(activeTeamProjects); + return currentItems + .filter((el) => projectMap.has(el.id)) + .map((el) => ({ + project: { + name: el.name, + imageUrl: el.imageUrl, + color: el.color + }, + status: el.status, + startDate: el.startDate, + endDate: el.endDate, + members: el.members, + managers: el.members, + teams: el.teams + })); +}, [currentItems, activeTeamProjects]);
33-40: Improve error handling for localStorage access.The error handling for localStorage access only logs to console. Consider providing a fallback mechanism and user notification.
Apply this diff to enhance error handling:
+const [storageError, setStorageError] = useState<string | null>(null); const lastSelectedView = useMemo(() => { try { return localStorage.getItem(LAST_SELECTED_PROJECTS_VIEW) as TViewMode; } catch (error) { console.error('Failed to access localStorage:', error); + setStorageError('Failed to load view preference'); return null; } }, []); + +useEffect(() => { + if (storageError) { + // Show error notification to user + toast.error(storageError); + } +}, [storageError]);apps/web/app/[locale]/projects/components/filters-card-modal.tsx (3)
26-31: Add loading states for data fetching.The component doesn't show loading states while fetching data, which could lead to a poor user experience.
Apply this diff to add loading states:
const { teams } = useOrganizationTeams(); -const { organizationProjects } = useOrganizationProjects(); +const { organizationProjects, loading: projectsLoading } = useOrganizationProjects(); const teamMembers = useMemo( () => organizationProjects.flatMap((project) => project.members ?? []), [organizationProjects] );Then update the UI to show loading states:
<MultiSelectWithSearch + disabled={projectsLoading} selectedOptions={selectedMembers.map((memberId) => { const name = members.find((member) => member.id === memberId)?.value ?? '-'; return { value: name, id: memberId }; })} onChange={(data) => setSelectedMembers(data.map((member) => member.id))} options={members} searchEnabled placeholder="Select a member..." />
28-31: Optimize teamMembers computation.The
teamMembersmemo flattens the entire project list on every render. Consider moving the filtering logic to the individual member/manager memos.Apply this diff to optimize the computation:
-const teamMembers = useMemo( - () => organizationProjects.flatMap((project) => project.members ?? []), - [organizationProjects] -); const members = useMemo( () => - teamMembers - ?.filter((el) => !el?.isManager) + organizationProjects + .flatMap((project) => project.members ?? []) + .filter((el) => !el?.isManager) ?.map((el) => ({ imageUrl: el?.employee?.user?.imageUrl, value: el?.employee?.fullName, id: el?.employeeId })) || [], - [teamMembers] + [organizationProjects] );
122-124: Add loading states for data fetching.The modal doesn't show loading states while fetching teams, projects, or task status data.
Add loading indicators to improve user experience:
<Modal className="w-[26rem]" isOpen={open} closeModal={closeModal}> <Card className="w-full border h-full " shadow="custom"> + {(getOrganizationProjectsLoading || getTeamsLoading || getTaskStatusLoading) && ( + <div className="absolute inset-0 bg-white/50 dark:bg-dark--theme/50 flex items-center justify-center"> + <SpinnerLoader /> + </div> + )} <div className="w-full flex gap-2 font-medium text-[1rem]">apps/web/app/[locale]/projects/components/multi-select-with-search.tsx (1)
45-70: Enhance accessibility for screen readers.The component lacks proper ARIA attributes for better screen reader support.
Apply this diff to improve accessibility:
<Popover.Button className={cn( 'border relative grow rounded-lg w-full bg-transparent text-xs px-3 py-2', searchEnabled && ' pl-9' )} + aria-haspopup="listbox" + aria-expanded="true" {...(searchEnabled ? { placeholder: placeholder, as: 'input', type: 'text', onChange: handleSearchChange, - value: searchTerm + value: searchTerm, + 'aria-label': `Search ${placeholder}` } : { as: 'button', - children: <div className="w-full text-xs text-slate-400 text-left">{placeholder}</div> + children: <div className="w-full text-xs text-slate-400 text-left" role="combobox">{placeholder}</div> })} ></Popover.Button>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
apps/web/app/[locale]/projects/components/data-table.tsx(1 hunks)apps/web/app/[locale]/projects/components/filters-card-modal.tsx(1 hunks)apps/web/app/[locale]/projects/components/multi-select-with-search.tsx(1 hunks)apps/web/app/[locale]/projects/components/page-component.tsx(1 hunks)apps/web/app/hooks/features/useOrganizationProjects.ts(3 hunks)apps/web/app/interfaces/IProject.ts(3 hunks)apps/web/app/services/client/api/organization-projects.ts(2 hunks)apps/web/components/nav-projects.tsx(2 hunks)apps/web/locales/ar.json(3 hunks)apps/web/locales/bg.json(3 hunks)apps/web/locales/de.json(3 hunks)apps/web/locales/en.json(3 hunks)apps/web/locales/es.json(2 hunks)apps/web/locales/fr.json(3 hunks)apps/web/locales/he.json(3 hunks)apps/web/locales/it.json(3 hunks)apps/web/locales/nl.json(3 hunks)apps/web/locales/pl.json(3 hunks)apps/web/locales/pt.json(3 hunks)apps/web/locales/ru.json(3 hunks)apps/web/locales/zh.json(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/web/app/hooks/features/useOrganizationProjects.ts
- apps/web/components/nav-projects.tsx
- apps/web/app/[locale]/projects/components/data-table.tsx
- apps/web/locales/zh.json
- apps/web/locales/nl.json
- apps/web/locales/ar.json
- apps/web/locales/ru.json
- apps/web/locales/it.json
- apps/web/locales/pt.json
- apps/web/locales/pl.json
- apps/web/locales/he.json
👮 Files not reviewed due to content moderation or server errors (4)
- apps/web/app/interfaces/IProject.ts
- apps/web/locales/en.json
- apps/web/locales/bg.json
- apps/web/locales/es.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: deploy
🔇 Additional comments (7)
apps/web/locales/de.json (3)
29-32: New Localization Strings in "common" Section
The new keys for"START_DATE","END_DATE","MEMBERS", and"MANAGERS"have been introduced with accurate German translations. They will support the enhanced project management views by providing the required labels.
324-329: Addition of Budget and Filter Keys
The diff adds several budget and filtering related keys:"COST_BASED","HOURS_BASED","MANAGER","BUDGET_TYPE","CLEAR_FILTERS", and"APPLY_FILTERS". The translations appear correct. Please verify that the distinction between"MANAGER"(singular) and"MANAGERS"(plural, see line 32) is consistently handled in the UI.
431-441: New "projects" Section for Project Management
A dedicated"projects"block has been added that includes a"projectTitle"object (with both plural and singular forms), the"CREATE_NEW_PROJECT"key, and a nested"views"object with"LIST_VIEW"and"GRID_VIEW". These additions will directly support the new grid and list view feature. Ensure that component implementations reference these keys for consistent localization.apps/web/locales/fr.json (3)
30-33: New Localization Strings in "common" Section
The new entries for"START_DATE": "Date de début","END_DATE": "Date de fin","MEMBERS": "Membres", and"MANAGERS": "Gestionnaires"are properly translated into French. They are important for the project management feature and seem consistent with the project's terminology.
252-257: Addition of Budget and Filter Keys
The changes introduce"COST_BASED","HOURS_BASED","MANAGER","BUDGET_TYPE","CLEAR_FILTERS", and"APPLY_FILTERS"in French. The translations ("Basé sur les coûts", "Basé sur les heures", etc.) are clear. Just ensure that the singular form"MANAGER"is used in the correct context versus any plural variants elsewhere.
428-438: New "projects" Block for Enhanced Views
The new"projects"section is added with a"projectTitle"object (providing both"PLURAL": "Projets"and"SINGULAR": "Projet"), as well as"CREATE_NEW_PROJECT"and a"views"object containing"LIST_VIEW": "Vue liste"and"GRID_VIEW": "Vue grille". These keys will support the grid and list view for projects. Confirm that the components rendering project-related information are updated to use these localization keys.apps/web/app/[locale]/projects/components/filters-card-modal.tsx (1)
78-98: 🛠️ Refactor suggestionAdd cleanup for URL updates.
The
handleApplyFiltersfunction updates the URL without cleaning up, which could lead to memory leaks. Consider using URLSearchParams constructor with the current params.Apply this diff to fix the URL handling:
const handleApplyFilters = useCallback(() => { - const searchParams = new URLSearchParams(window.location.search); + const searchParams = new URLSearchParams(params.toString()); const updateQueryParam = (key: string, values: string[]) => { if (values.length > 0) { searchParams.set(key, values.join(',')); } else { searchParams.delete(key); } }; updateQueryParam('teams', selectedTeams); updateQueryParam('members', selectedMembers); updateQueryParam('managers', selectedManagers); updateQueryParam('status', selectedStatus); updateQueryParam('budgetTypes', selectedBudgetType); closeModal(); router.replace(`?${searchParams.toString()}`, { scroll: false }); -}, [selectedTeams, selectedMembers, selectedManagers, selectedStatus, selectedBudgetType, closeModal, router]); +}, [selectedTeams, selectedMembers, selectedManagers, selectedStatus, selectedBudgetType, closeModal, router, params]);Likely an incorrect or invalid review comment.
| const handleSelect = useCallback( | ||
| (selectedOption: T) => { | ||
| const newSelectedOptions = selectedOptions; | ||
|
|
||
| if (!selectedOptions.map((el) => el.value).includes(selectedOption.value)) { | ||
| newSelectedOptions.push(selectedOption); | ||
| } else { | ||
| newSelectedOptions.splice(newSelectedOptions.indexOf(selectedOption), 1); | ||
| } | ||
| onChange(newSelectedOptions); | ||
| }, | ||
| [onChange, selectedOptions] | ||
| ); |
There was a problem hiding this comment.
Fix state mutation in handleSelect.
The handleSelect function directly mutates the selectedOptions array. This can lead to unexpected behavior in React.
Apply this diff to fix the state mutation:
const handleSelect = useCallback(
(selectedOption: T) => {
- const newSelectedOptions = selectedOptions;
- if (!selectedOptions.map((el) => el.value).includes(selectedOption.value)) {
- newSelectedOptions.push(selectedOption);
- } else {
- newSelectedOptions.splice(newSelectedOptions.indexOf(selectedOption), 1);
- }
- onChange(newSelectedOptions);
+ const isSelected = selectedOptions.map((el) => el.value).includes(selectedOption.value);
+ const newSelectedOptions = isSelected
+ ? selectedOptions.filter((option) => option.value !== selectedOption.value)
+ : [...selectedOptions, selectedOption];
+ onChange(newSelectedOptions);
},
[onChange, selectedOptions]
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleSelect = useCallback( | |
| (selectedOption: T) => { | |
| const newSelectedOptions = selectedOptions; | |
| if (!selectedOptions.map((el) => el.value).includes(selectedOption.value)) { | |
| newSelectedOptions.push(selectedOption); | |
| } else { | |
| newSelectedOptions.splice(newSelectedOptions.indexOf(selectedOption), 1); | |
| } | |
| onChange(newSelectedOptions); | |
| }, | |
| [onChange, selectedOptions] | |
| ); | |
| const handleSelect = useCallback( | |
| (selectedOption: T) => { | |
| const isSelected = selectedOptions.map((el) => el.value).includes(selectedOption.value); | |
| const newSelectedOptions = isSelected | |
| ? selectedOptions.filter((option) => option.value !== selectedOption.value) | |
| : [...selectedOptions, selectedOption]; | |
| onChange(newSelectedOptions); | |
| }, | |
| [onChange, selectedOptions] | |
| ); |
Fix state mutation in handleSelect.
The handleSelect function directly mutates the selectedOptions array, which can lead to unexpected behavior in React. Instead, create a new array to maintain immutability.
Apply this diff to fix the state mutation:
const handleSelect = useCallback(
(selectedOption: T) => {
- const newSelectedOptions = selectedOptions;
+ const newSelectedOptions = [...selectedOptions];
if (!selectedOptions.map((el) => el.value).includes(selectedOption.value)) {
newSelectedOptions.push(selectedOption);
} else {
newSelectedOptions.splice(newSelectedOptions.indexOf(selectedOption), 1);
}
onChange(newSelectedOptions);
},
[onChange, selectedOptions]
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleSelect = useCallback( | |
| (selectedOption: T) => { | |
| const newSelectedOptions = selectedOptions; | |
| if (!selectedOptions.map((el) => el.value).includes(selectedOption.value)) { | |
| newSelectedOptions.push(selectedOption); | |
| } else { | |
| newSelectedOptions.splice(newSelectedOptions.indexOf(selectedOption), 1); | |
| } | |
| onChange(newSelectedOptions); | |
| }, | |
| [onChange, selectedOptions] | |
| ); | |
| const handleSelect = useCallback( | |
| (selectedOption: T) => { | |
| const newSelectedOptions = [...selectedOptions]; | |
| if (!selectedOptions.map((el) => el.value).includes(selectedOption.value)) { | |
| newSelectedOptions.push(selectedOption); | |
| } else { | |
| newSelectedOptions.splice(newSelectedOptions.indexOf(selectedOption), 1); | |
| } | |
| onChange(newSelectedOptions); | |
| }, | |
| [onChange, selectedOptions] | |
| ); |
| return ( | ||
| <Popover className={'relative '}> | ||
| <div className="w-full flex "> | ||
| {searchEnabled && ( | ||
| <Search size={15} className=" text-slate-400 z-10 absolute top-1/2 -translate-y-1/2 left-3" /> | ||
| )} | ||
| <Popover.Button | ||
| className={cn( | ||
| 'border relative grow rounded-lg w-full bg-transparent text-xs px-3 py-2', | ||
| searchEnabled && ' pl-9' | ||
| )} | ||
| {...(searchEnabled | ||
| ? { | ||
| placeholder: placeholder, | ||
| as: 'input', | ||
| type: 'text', | ||
| onChange: handleSearchChange, | ||
| value: searchTerm | ||
| } | ||
| : { | ||
| as: 'button', | ||
| children: <div className="w-full text-xs text-slate-400 text-left">{placeholder}</div> | ||
| })} | ||
| ></Popover.Button> | ||
| <ChevronDown size={20} className=" text-slate-400 absolute top-1/2 -translate-y-1/2 right-3" /> | ||
| </div> | ||
| <Popover.Panel className={'absolute w-full mt-1 z-20 bg-white dark:bg-gray-800 rounded-md shadow-md'}> | ||
| <ul className="w-full flex flex-col h-52 gap-[.125rem] p-[.125rem]"> | ||
| <ScrollArea> | ||
| {options | ||
| ?.filter((option) => String(option.value).toLowerCase().includes(searchTerm.toLowerCase())) | ||
| ?.map((option) => { | ||
| const isSelected = selectedOptions.map((el) => el.value).includes(option.value); | ||
|
|
||
| return ( | ||
| <li | ||
| onClick={() => handleSelect(option)} | ||
| key={option.id} | ||
| className={cn( | ||
| 'w-full cursor-pointer flex gap-2 px-4 py-2 rounded-sm text-sm', | ||
| isSelected ? 'bg-primary text-white' : 'hover:bg-primary/10' | ||
| )} | ||
| > | ||
| <Check | ||
| className={cn( | ||
| selectedOptions.length > 0 ? 'block' : 'hidden', | ||
| isSelected ? ' opacity-100' : ' opacity-0' | ||
| )} | ||
| size={15} | ||
| /> | ||
| <span className=" text-xs grow truncate overflow-hidden whitespace-nowrap "> | ||
| {String(option.value)} | ||
| </span> | ||
| </li> | ||
| ); | ||
| })} | ||
|
|
||
| <ScrollBar /> | ||
| </ScrollArea> | ||
| </ul> | ||
| </Popover.Panel> | ||
| </Popover> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add accessibility attributes for better screen reader support.
The component lacks proper ARIA attributes for accessibility.
Apply this diff to add accessibility attributes:
<Popover className={'relative '}>
<div className="w-full flex ">
{searchEnabled && (
<Search size={15} className=" text-slate-400 z-10 absolute top-1/2 -translate-y-1/2 left-3" />
)}
<Popover.Button
className={cn(
'border relative grow rounded-lg w-full bg-transparent text-xs px-3 py-2',
searchEnabled && ' pl-9'
)}
+ aria-label={placeholder}
+ aria-expanded="true"
+ role="combobox"
+ aria-controls="options-list"
+ aria-multiselectable="true"
{...(searchEnabled
? {
placeholder: placeholder,
as: 'input',
type: 'text',
onChange: handleSearchChange,
value: searchTerm
}
: {
as: 'button',
children: <div className="w-full text-xs text-slate-400 text-left">{placeholder}</div>
})}
></Popover.Button>
<ChevronDown size={20} className=" text-slate-400 absolute top-1/2 -translate-y-1/2 right-3" />
</div>
<Popover.Panel className={'absolute w-full mt-1 z-20 bg-white dark:bg-gray-800 rounded-md shadow-md'}>
- <ul className="w-full flex flex-col h-52 gap-[.125rem] p-[.125rem]">
+ <ul
+ id="options-list"
+ role="listbox"
+ aria-label={`${placeholder} options`}
+ className="w-full flex flex-col h-52 gap-[.125rem] p-[.125rem]"
+ >
<ScrollArea>
{options
?.filter((option) => String(option.value).toLowerCase().includes(searchTerm.toLowerCase()))
?.map((option) => {
const isSelected = selectedOptions.map((el) => el.value).includes(option.value);
return (
<li
+ role="option"
+ aria-selected={isSelected}
onClick={() => handleSelect(option)}
key={option.id}
className={cn(
'w-full cursor-pointer flex gap-2 px-4 py-2 rounded-sm text-sm',
isSelected ? 'bg-primary text-white' : 'hover:bg-primary/10'
)}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <Popover className={'relative '}> | |
| <div className="w-full flex "> | |
| {searchEnabled && ( | |
| <Search size={15} className=" text-slate-400 z-10 absolute top-1/2 -translate-y-1/2 left-3" /> | |
| )} | |
| <Popover.Button | |
| className={cn( | |
| 'border relative grow rounded-lg w-full bg-transparent text-xs px-3 py-2', | |
| searchEnabled && ' pl-9' | |
| )} | |
| {...(searchEnabled | |
| ? { | |
| placeholder: placeholder, | |
| as: 'input', | |
| type: 'text', | |
| onChange: handleSearchChange, | |
| value: searchTerm | |
| } | |
| : { | |
| as: 'button', | |
| children: <div className="w-full text-xs text-slate-400 text-left">{placeholder}</div> | |
| })} | |
| ></Popover.Button> | |
| <ChevronDown size={20} className=" text-slate-400 absolute top-1/2 -translate-y-1/2 right-3" /> | |
| </div> | |
| <Popover.Panel className={'absolute w-full mt-1 z-20 bg-white dark:bg-gray-800 rounded-md shadow-md'}> | |
| <ul className="w-full flex flex-col h-52 gap-[.125rem] p-[.125rem]"> | |
| <ScrollArea> | |
| {options | |
| ?.filter((option) => String(option.value).toLowerCase().includes(searchTerm.toLowerCase())) | |
| ?.map((option) => { | |
| const isSelected = selectedOptions.map((el) => el.value).includes(option.value); | |
| return ( | |
| <li | |
| onClick={() => handleSelect(option)} | |
| key={option.id} | |
| className={cn( | |
| 'w-full cursor-pointer flex gap-2 px-4 py-2 rounded-sm text-sm', | |
| isSelected ? 'bg-primary text-white' : 'hover:bg-primary/10' | |
| )} | |
| > | |
| <Check | |
| className={cn( | |
| selectedOptions.length > 0 ? 'block' : 'hidden', | |
| isSelected ? ' opacity-100' : ' opacity-0' | |
| )} | |
| size={15} | |
| /> | |
| <span className=" text-xs grow truncate overflow-hidden whitespace-nowrap "> | |
| {String(option.value)} | |
| </span> | |
| </li> | |
| ); | |
| })} | |
| <ScrollBar /> | |
| </ScrollArea> | |
| </ul> | |
| </Popover.Panel> | |
| </Popover> | |
| return ( | |
| <Popover className={'relative '}> | |
| <div className="w-full flex "> | |
| {searchEnabled && ( | |
| <Search size={15} className=" text-slate-400 z-10 absolute top-1/2 -translate-y-1/2 left-3" /> | |
| )} | |
| <Popover.Button | |
| className={cn( | |
| 'border relative grow rounded-lg w-full bg-transparent text-xs px-3 py-2', | |
| searchEnabled && ' pl-9' | |
| )} | |
| aria-label={placeholder} | |
| aria-expanded="true" | |
| role="combobox" | |
| aria-controls="options-list" | |
| aria-multiselectable="true" | |
| {...(searchEnabled | |
| ? { | |
| placeholder: placeholder, | |
| as: 'input', | |
| type: 'text', | |
| onChange: handleSearchChange, | |
| value: searchTerm | |
| } | |
| : { | |
| as: 'button', | |
| children: <div className="w-full text-xs text-slate-400 text-left">{placeholder}</div> | |
| })} | |
| ></Popover.Button> | |
| <ChevronDown size={20} className=" text-slate-400 absolute top-1/2 -translate-y-1/2 right-3" /> | |
| </div> | |
| <Popover.Panel className={'absolute w-full mt-1 z-20 bg-white dark:bg-gray-800 rounded-md shadow-md'}> | |
| <ul | |
| id="options-list" | |
| role="listbox" | |
| aria-label={`${placeholder} options`} | |
| className="w-full flex flex-col h-52 gap-[.125rem] p-[.125rem]" | |
| > | |
| <ScrollArea> | |
| {options | |
| ?.filter((option) => String(option.value).toLowerCase().includes(searchTerm.toLowerCase())) | |
| ?.map((option) => { | |
| const isSelected = selectedOptions.map((el) => el.value).includes(option.value); | |
| return ( | |
| <li | |
| role="option" | |
| aria-selected={isSelected} | |
| onClick={() => handleSelect(option)} | |
| key={option.id} | |
| className={cn( | |
| 'w-full cursor-pointer flex gap-2 px-4 py-2 rounded-sm text-sm', | |
| isSelected ? 'bg-primary text-white' : 'hover:bg-primary/10' | |
| )} | |
| > | |
| <Check | |
| className={cn( | |
| selectedOptions.length > 0 ? 'block' : 'hidden', | |
| isSelected ? ' opacity-100' : ' opacity-0' | |
| )} | |
| size={15} | |
| /> | |
| <span className=" text-xs grow truncate overflow-hidden whitespace-nowrap "> | |
| {String(option.value)} | |
| </span> | |
| </li> | |
| ); | |
| })} | |
| <ScrollBar /> | |
| </ScrollArea> | |
| </ul> | |
| </Popover.Panel> | |
| </Popover> | |
| ); |
| <Paginate | ||
| total={total} | ||
| onPageChange={onPageChange} | ||
| pageCount={1} // Set Static to 1 - It will be calculated dynamically in Paginate component | ||
| itemsPerPage={itemsPerPage} | ||
| itemOffset={itemOffset} | ||
| endOffset={endOffset} | ||
| setItemsPerPage={setItemsPerPage} | ||
| className="pt-0" | ||
| /> |
There was a problem hiding this comment.
Fix hardcoded page count in pagination.
The page count is hardcoded to 1, which could lead to incorrect pagination behavior.
Apply this diff to calculate the page count dynamically:
<Paginate
total={total}
onPageChange={onPageChange}
- pageCount={1} // Set Static to 1 - It will be calculated dynamically in Paginate component
+ pageCount={Math.ceil(total / itemsPerPage)}
itemsPerPage={itemsPerPage}
itemOffset={itemOffset}
endOffset={endOffset}
setItemsPerPage={setItemsPerPage}
className="pt-0"
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Paginate | |
| total={total} | |
| onPageChange={onPageChange} | |
| pageCount={1} // Set Static to 1 - It will be calculated dynamically in Paginate component | |
| itemsPerPage={itemsPerPage} | |
| itemOffset={itemOffset} | |
| endOffset={endOffset} | |
| setItemsPerPage={setItemsPerPage} | |
| className="pt-0" | |
| /> | |
| <Paginate | |
| total={total} | |
| onPageChange={onPageChange} | |
| pageCount={Math.ceil(total / itemsPerPage)} | |
| itemsPerPage={itemsPerPage} | |
| itemOffset={itemOffset} | |
| endOffset={endOffset} | |
| setItemsPerPage={setItemsPerPage} | |
| className="pt-0" | |
| /> |
| const handleApplyFilters = useCallback(() => { | ||
| const searchParams = new URLSearchParams(window.location.search); | ||
|
|
||
| const updateQueryParam = (key: string, values: string[]) => { | ||
| if (values.length > 0) { | ||
| searchParams.set(key, values.join(',')); | ||
| } else { | ||
| searchParams.delete(key); | ||
| } | ||
| }; | ||
|
|
||
| updateQueryParam('teams', selectedTeams); | ||
| updateQueryParam('members', selectedMembers); | ||
| updateQueryParam('managers', selectedManagers); | ||
| updateQueryParam('status', selectedStatus); | ||
| updateQueryParam('budgetTypes', selectedBudgetType); | ||
| closeModal(); | ||
|
|
||
| router.replace(`?${searchParams.toString()}`, { scroll: false }); | ||
| }, [selectedTeams, selectedMembers, selectedManagers, selectedStatus, selectedBudgetType, closeModal, router]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for URL manipulation.
The handleApplyFilters function doesn't handle errors that could occur during URL manipulation.
Apply this diff to add error handling:
const handleApplyFilters = useCallback(() => {
+ try {
const searchParams = new URLSearchParams(window.location.search);
const updateQueryParam = (key: string, values: string[]) => {
if (values.length > 0) {
searchParams.set(key, values.join(','));
} else {
searchParams.delete(key);
}
};
updateQueryParam('teams', selectedTeams);
updateQueryParam('members', selectedMembers);
updateQueryParam('managers', selectedManagers);
updateQueryParam('status', selectedStatus);
updateQueryParam('budgetTypes', selectedBudgetType);
closeModal();
router.replace(`?${searchParams.toString()}`, { scroll: false });
+ } catch (error) {
+ console.error('Failed to apply filters:', error);
+ // TODO: Show error notification to user
+ }
}, [selectedTeams, selectedMembers, selectedManagers, selectedStatus, selectedBudgetType, closeModal, router]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const handleApplyFilters = useCallback(() => { | |
| const searchParams = new URLSearchParams(window.location.search); | |
| const updateQueryParam = (key: string, values: string[]) => { | |
| if (values.length > 0) { | |
| searchParams.set(key, values.join(',')); | |
| } else { | |
| searchParams.delete(key); | |
| } | |
| }; | |
| updateQueryParam('teams', selectedTeams); | |
| updateQueryParam('members', selectedMembers); | |
| updateQueryParam('managers', selectedManagers); | |
| updateQueryParam('status', selectedStatus); | |
| updateQueryParam('budgetTypes', selectedBudgetType); | |
| closeModal(); | |
| router.replace(`?${searchParams.toString()}`, { scroll: false }); | |
| }, [selectedTeams, selectedMembers, selectedManagers, selectedStatus, selectedBudgetType, closeModal, router]); | |
| const handleApplyFilters = useCallback(() => { | |
| try { | |
| const searchParams = new URLSearchParams(window.location.search); | |
| const updateQueryParam = (key: string, values: string[]) => { | |
| if (values.length > 0) { | |
| searchParams.set(key, values.join(',')); | |
| } else { | |
| searchParams.delete(key); | |
| } | |
| }; | |
| updateQueryParam('teams', selectedTeams); | |
| updateQueryParam('members', selectedMembers); | |
| updateQueryParam('managers', selectedManagers); | |
| updateQueryParam('status', selectedStatus); | |
| updateQueryParam('budgetTypes', selectedBudgetType); | |
| closeModal(); | |
| router.replace(`?${searchParams.toString()}`, { scroll: false }); | |
| } catch (error) { | |
| console.error('Failed to apply filters:', error); | |
| // TODO: Show error notification to user | |
| } | |
| }, [selectedTeams, selectedMembers, selectedManagers, selectedStatus, selectedBudgetType, closeModal, router]); |
| const getSelectedQueries = useCallback(() => { | ||
| setSelectedTeams(params.get('teams')?.split(',') || []); | ||
| setSelectedMembers(params.get('members')?.split(',') || []); | ||
| setSelectedManagers(params.get('managers')?.split(',') || []); | ||
| setSelectedStatus(params.get('status')?.split(',') || []); | ||
| setSelectedBudgetType(params.get('budgetTypes')?.split(',') || []); | ||
| }, [params]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for selected filters.
The getSelectedQueries function doesn't validate the filter values before setting them.
Apply this diff to add validation:
const getSelectedQueries = useCallback(() => {
+ const validateIds = (ids: string[] = [], validIds: string[]) => {
+ return ids.filter(id => validIds.includes(id));
+ };
+
+ const teamIds = teams.map(team => team.id);
+ const memberIds = members.map(member => member.id);
+ const managerIds = managers.map(manager => manager.id);
+ const statusIds = taskStatus.map(status => status.id);
+ const budgetTypeIds = budgetTypes.map(type => type.id);
+
- setSelectedTeams(params.get('teams')?.split(',') || []);
- setSelectedMembers(params.get('members')?.split(',') || []);
- setSelectedManagers(params.get('managers')?.split(',') || []);
- setSelectedStatus(params.get('status')?.split(',') || []);
- setSelectedBudgetType(params.get('budgetTypes')?.split(',') || []);
+ setSelectedTeams(validateIds(params.get('teams')?.split(','), teamIds));
+ setSelectedMembers(validateIds(params.get('members')?.split(','), memberIds));
+ setSelectedManagers(validateIds(params.get('managers')?.split(','), managerIds));
+ setSelectedStatus(validateIds(params.get('status')?.split(','), statusIds));
+ setSelectedBudgetType(validateIds(params.get('budgetTypes')?.split(','), budgetTypeIds));
}, [params]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getSelectedQueries = useCallback(() => { | |
| setSelectedTeams(params.get('teams')?.split(',') || []); | |
| setSelectedMembers(params.get('members')?.split(',') || []); | |
| setSelectedManagers(params.get('managers')?.split(',') || []); | |
| setSelectedStatus(params.get('status')?.split(',') || []); | |
| setSelectedBudgetType(params.get('budgetTypes')?.split(',') || []); | |
| }, [params]); | |
| const getSelectedQueries = useCallback(() => { | |
| const validateIds = (ids: string[] = [], validIds: string[]) => { | |
| return ids.filter(id => validIds.includes(id)); | |
| }; | |
| const teamIds = teams.map(team => team.id); | |
| const memberIds = members.map(member => member.id); | |
| const managerIds = managers.map(manager => manager.id); | |
| const statusIds = taskStatus.map(status => status.id); | |
| const budgetTypeIds = budgetTypes.map(type => type.id); | |
| setSelectedTeams(validateIds(params.get('teams')?.split(','), teamIds)); | |
| setSelectedMembers(validateIds(params.get('members')?.split(','), memberIds)); | |
| setSelectedManagers(validateIds(params.get('managers')?.split(','), managerIds)); | |
| setSelectedStatus(validateIds(params.get('status')?.split(','), statusIds)); | |
| setSelectedBudgetType(validateIds(params.get('budgetTypes')?.split(','), budgetTypeIds)); | |
| }, [params]); |
| const filteredProjects = useMemo( | ||
| () => | ||
| currentItems | ||
| ?.filter((el) => activeTeamProjects.includes(el.id)) | ||
| ?.map((el) => ({ | ||
| project: { | ||
| name: el.name, | ||
| imageUrl: el.imageUrl, | ||
| color: el.color | ||
| }, | ||
| status: el.status, | ||
| startDate: el.startDate, | ||
| endDate: el.endDate, | ||
| members: el.members, | ||
| managers: el.members, | ||
| teams: el.teams | ||
| })), | ||
| [currentItems, activeTeamProjects] | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix incorrect mapping of managers and optimize filteredProjects.
The filteredProjects memo has two issues:
- Managers are incorrectly mapped to members (
managers: el.members). - The mapping operation could be optimized by moving the filter outside the memo.
Apply this diff to fix the issues:
+const filteredCurrentItems = useMemo(
+ () => currentItems?.filter((el) => activeTeamProjects.includes(el.id)),
+ [currentItems, activeTeamProjects]
+);
const filteredProjects = useMemo(
() =>
- currentItems
- ?.filter((el) => activeTeamProjects.includes(el.id))
+ filteredCurrentItems
?.map((el) => ({
project: {
name: el.name,
imageUrl: el.imageUrl,
color: el.color
},
status: el.status,
startDate: el.startDate,
endDate: el.endDate,
members: el.members,
- managers: el.members,
+ managers: el.members?.filter(member => member.isManager),
teams: el.teams
})),
- [currentItems, activeTeamProjects]
+ [filteredCurrentItems]
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const filteredProjects = useMemo( | |
| () => | |
| currentItems | |
| ?.filter((el) => activeTeamProjects.includes(el.id)) | |
| ?.map((el) => ({ | |
| project: { | |
| name: el.name, | |
| imageUrl: el.imageUrl, | |
| color: el.color | |
| }, | |
| status: el.status, | |
| startDate: el.startDate, | |
| endDate: el.endDate, | |
| members: el.members, | |
| managers: el.members, | |
| teams: el.teams | |
| })), | |
| [currentItems, activeTeamProjects] | |
| ); | |
| const filteredCurrentItems = useMemo( | |
| () => currentItems?.filter((el) => activeTeamProjects.includes(el.id)), | |
| [currentItems, activeTeamProjects] | |
| ); | |
| const filteredProjects = useMemo( | |
| () => | |
| filteredCurrentItems?.map((el) => ({ | |
| project: { | |
| name: el.name, | |
| imageUrl: el.imageUrl, | |
| color: el.color | |
| }, | |
| status: el.status, | |
| startDate: el.startDate, | |
| endDate: el.endDate, | |
| members: el.members, | |
| managers: el.members?.filter((member) => member.isManager), | |
| teams: el.teams | |
| })), | |
| [filteredCurrentItems] | |
| ); |
Description
Closes #3311
Type of Change
Checklist
Current screenshots
Loom
Summary by CodeRabbit
New Features
DataTableProjectcomponent for structured project data display with sorting and filtering capabilities.GridItemcomponent for visually appealing project information presentation.Refactor