Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[WEB - 2779] feat: Added sort order for issue activity #6087

Open
wants to merge 3 commits into
base: preview
Choose a base branch
from
+42 −2
Open

[WEB - 2779] feat: Added sort order for issue activity #6087

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
67353ae
added sort order for issue activity
mathalav55 Nov 22, 2024
51619a7
fixed invalid date generation issue
mathalav55 Nov 22, 2024
1a58cdf
fixed lint errors, optimized code
mathalav55 Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions web/ce/components/issues/worklog/activity/sort-root.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
"use client"

import { FC } from "react";
import { ArrowUpWideNarrow, ArrowDownWideNarrow } from "lucide-react";
import { getButtonStyling } from "@plane/ui";
// helpers
import { cn } from '@/helpers/common.helper'

export type TActivitySortRoot = {
sortOrder: 'asc' | 'desc'
toggleSort: () => void
}
export const ActivitySortRoot:FC<TActivitySortRoot> = (props)=>{

Check failure on line 13 in web/ce/components/issues/worklog/activity/sort-root.tsx

View workflow job for this annotation

GitHub Actions / lint-web 

Unexpected block statement surrounding arrow body; move the returned value immediately after the `=>`
return <div className={cn(getButtonStyling("neutral-primary", "sm"), "px-2 text-custom-text-300 cursor-pointer")} onClick={()=>{props.toggleSort()}}>
{
props.sortOrder === 'asc' ? <ArrowUpWideNarrow className="size-4 " /> : <ArrowDownWideNarrow className="size-4 " />
}
</div>
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance component implementation for accessibility and performance.

The component needs improvements in accessibility and could benefit from performance optimizations.

-export const ActivitySortRoot:FC<TActivitySortRoot> = (props)=>{
+import { memo } from 'react';
+
+export const ActivitySortRoot: FC<TActivitySortRoot> = memo((props) => {
+  const { sortOrder, toggleSort } = props;
+  const label = `Sort activities ${sortOrder === 'asc' ? 'descending' : 'ascending'}`;
+
   return (
     <div 
-      className={cn(getButtonStyling("neutral-primary", "sm"), "px-2 text-custom-text-300 cursor-pointer")} 
-      onClick={()=>{props.toggleSort()}}
+      className={cn(getButtonStyling("neutral-primary", "sm"), "px-2 text-custom-text-300 cursor-pointer")}
+      onClick={toggleSort}
+      role="button"
+      aria-label={label}
+      tabIndex={0}
+      onKeyDown={(e) => e.key === 'Enter' && toggleSort()}
     >
       {
-        props.sortOrder === 'asc' ? <ArrowUpWideNarrow className="size-4 " /> : <ArrowDownWideNarrow className="size-4 " />
+        sortOrder === 'asc' ? <ArrowUpWideNarrow className="size-4" /> : <ArrowDownWideNarrow className="size-4" />
       }
     </div>
-}
+});
+
+ActivitySortRoot.displayName = 'ActivitySortRoot';

Changes include:

  1. Added proper accessibility attributes (role, aria-label, keyboard support)
  2. Memoized component for better performance
  3. Destructured props for cleaner code
  4. Fixed trailing space in className
  5. Added display name for better debugging
📝 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.

Suggested change
export const ActivitySortRoot:FC<TActivitySortRoot> = (props)=>{
return <div className={cn(getButtonStyling("neutral-primary", "sm"), "px-2 text-custom-text-300 cursor-pointer")} onClick={()=>{props.toggleSort()}}>
{
props.sortOrder === 'asc' ? <ArrowUpWideNarrow className="size-4 " /> : <ArrowDownWideNarrow className="size-4 " />
}
</div>
}
import { memo } from 'react';
export const ActivitySortRoot: FC<TActivitySortRoot> = memo((props) => {
const { sortOrder, toggleSort } = props;
const label = `Sort activities ${sortOrder === 'asc' ? 'descending' : 'ascending'}`;
return (
<div
className={cn(getButtonStyling("neutral-primary", "sm"), "px-2 text-custom-text-300 cursor-pointer")}
onClick={toggleSort}
role="button"
aria-label={label}
tabIndex={0}
onKeyDown={(e) => e.key === 'Enter' && toggleSort()}
>
{
sortOrder === 'asc' ? <ArrowUpWideNarrow className="size-4" /> : <ArrowDownWideNarrow className="size-4" />
}
</div>
);
});
ActivitySortRoot.displayName = 'ActivitySortRoot';

15 changes: 12 additions & 3 deletions web/ce/store/issue/issue-details/activity.store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import concat from "lodash/concat";
import set from "lodash/set";
import sortBy from "lodash/sortBy";
import orderBy from "lodash/orderBy";

Check failure on line 5 in web/ce/store/issue/issue-details/activity.store.ts

View workflow job for this annotation

GitHub Actions / lint-web 

`lodash/orderBy` import should occur before import of `lodash/set`
import uniq from "lodash/uniq";
import update from "lodash/update";
import { action, makeObservable, observable, runInAction } from "mobx";
Expand All @@ -29,17 +29,20 @@

export interface IIssueActivityStore extends IIssueActivityStoreActions {
// observables
sortOrder: 'asc' | 'desc'
loader: TActivityLoader;
activities: TIssueActivityIdMap;
activityMap: TIssueActivityMap;
// helper methods
getActivitiesByIssueId: (issueId: string) => string[] | undefined;
getActivityById: (activityId: string) => TIssueActivity | undefined;
getActivityCommentByIssueId: (issueId: string) => TIssueActivityComment[] | undefined;
toggleSortOrder: ()=>void;
}

export class IssueActivityStore implements IIssueActivityStore {
// observables
sortOrder: "asc" | "desc" = 'asc';
loader: TActivityLoader = "fetch";
activities: TIssueActivityIdMap = {};
activityMap: TIssueActivityMap = {};
Expand All @@ -50,11 +53,13 @@
constructor(protected store: CoreRootStore) {
makeObservable(this, {
// observables
sortOrder: observable.ref,
loader: observable.ref,
activities: observable,
activityMap: observable,
// actions
fetchActivities: action,
toggleSortOrder: action
});
// services
this.issueActivityService = new IssueActivityService();
Expand Down Expand Up @@ -98,12 +103,16 @@
created_at: comment.created_at,
});
});

activityComments = sortBy(activityComments, "created_at");

Check failure on line 106 in web/ce/store/issue/issue-details/activity.store.ts

View workflow job for this annotation

GitHub Actions / lint-web 

Trailing spaces not allowed
activityComments = orderBy(activityComments, (e)=>new Date(e.created_at||''), this.sortOrder);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve date handling in sorting.

The current date handling could be problematic:

  1. Using an empty string as fallback for created_at will create an invalid date.
  2. The type suggests created_at is optional but it should be required for activities and comments.

Consider this improvement:

-    activityComments = orderBy(activityComments, (e)=>new Date(e.created_at||''), this.sortOrder);
+    activityComments = orderBy(activityComments, 'created_at', this.sortOrder);

Also, remove the empty line 106 for better code organization.

📝 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.

Suggested change
activityComments = orderBy(activityComments, (e)=>new Date(e.created_at||''), this.sortOrder);
activityComments = orderBy(activityComments, 'created_at', this.sortOrder);


return activityComments;
});

toggleSortOrder = ()=>{
this.sortOrder = this.sortOrder === 'asc' ? 'desc' : 'asc';
}

// actions
public async fetchActivities(
workspaceSlug: string,
Expand Down
3 changes: 3 additions & 0 deletions web/core/components/issues/issue-detail/issue-activity/root.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import { EUserPermissions } from "@/plane-web/constants/user-permissions";
// services
import { FileService } from "@/services/file.service";
import { ActivitySortRoot } from "@/plane-web/components/issues/worklog/activity/sort-root";

Check failure on line 22 in web/core/components/issues/issue-detail/issue-activity/root.tsx

View workflow job for this annotation

GitHub Actions / lint-web 

`@/plane-web/components/issues/worklog/activity/sort-root` import should occur before import of `@/plane-web/constants/issues`
const fileService = new FileService();

type TIssueActivity = {
Expand All @@ -43,6 +44,7 @@
// hooks
const {
issue: { getIssueById },
activity: { sortOrder, toggleSortOrder},
createComment,
updateComment,
removeComment,
Expand Down Expand Up @@ -162,6 +164,7 @@
disabled={disabled}
/>
)}
<ActivitySortRoot sortOrder={sortOrder} toggleSort={toggleSortOrder}/>
<ActivityFilterRoot
selectedFilters={selectedFilters}
toggleFilter={toggleFilter}
Expand Down
Loading