fix(PM-580): dont show action button when invite is inprogress #1087
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| import { assignCopilotOpportunity, copilotBaseUrl } from '~/apps/copilots/src/services/copilot-opportunities' | ||
| import { CopilotApplication, CopilotApplicationStatus } from '~/apps/copilots/src/models/CopilotApplication' | ||
| import { IconSolid, Tooltip } from '~/libs/ui' | ||
|
|
||
| import styles from './styles.module.scss' | ||
|
|
||
| const CopilotApplicationAction = (copilotApplication: CopilotApplication): JSX.Element => { | ||
| const CopilotApplicationAction = (copilotApplication: CopilotApplication, allCopilotApplications: CopilotApplication[]): JSX.Element => { |
There was a problem hiding this comment.
Suggestion
Consider renaming the isInvited variable to something more descriptive, such as isAnyApplicationInvited, to better convey that it checks the status of all copilot applications.
| const { opportunityId }: {opportunityId?: string} = useParams<{ opportunityId?: string }>() | ||
| const isInvited = useMemo(() => allCopilotApplications.findIndex(item => item.status === CopilotApplicationStatus.INVITED) > -1, [allCopilotApplications]) |
There was a problem hiding this comment.
Suggestion
The useMemo hook is used here to compute isInvited. Ensure that the computation is not overly complex or expensive, as useMemo should be used primarily for performance optimization in cases where the computation is costly.
| @@ -39,7 +40,7 @@ const CopilotApplicationAction = (copilotApplication: CopilotApplication): JSX.E | |||
| } | |||
|
|
|||
| { | |||
| copilotApplication.status === CopilotApplicationStatus.PENDING && ( | |||
| !isInvited && copilotApplication.status === CopilotApplicationStatus.PENDING && ( | |||
There was a problem hiding this comment.
Consider adding a check for isInvited before copilotApplication.status === CopilotApplicationStatus.PENDING to improve readability and ensure the logical flow is clear. This helps in understanding that the invitation status is checked first before evaluating the application status.
| @@ -177,6 +177,7 @@ const Table: <T extends { [propertyName: string]: any }>(props: TableProps<T>) = | |||
| <TableRow | |||
| key={getKey(index)} | |||
| data={sorted} | |||
| allRows={sortedData} | |||
There was a problem hiding this comment.
The addition of the allRows prop to the TableRow component should be verified to ensure that it is necessary for the functionality described in the pull request. If allRows is not used within TableRow, consider removing it to avoid unnecessary prop passing.
| @@ -16,13 +16,14 @@ interface TableCellProps<T> { | |||
| readonly index: number | |||
| readonly propertyName?: string | |||
| readonly className?: string | |||
| readonly renderer?: (data: T) => JSX.Element | undefined | |||
| readonly renderer?: (data: T, allRows?: ReadonlyArray<T>) => JSX.Element | undefined | |||
There was a problem hiding this comment.
The renderer function signature has been updated to include an optional allRows parameter. Ensure that all usages of this function are updated accordingly to handle this new parameter, even if it's not used in all cases.
| readonly type: TableCellType | ||
| readonly onExpand?: () => void | ||
| readonly as?: ElementType | ||
| readonly showExpandIndicator?: boolean | ||
| readonly isExpanded?: boolean | ||
| readonly colSpan?: number | ||
| allRows?: ReadonlyArray<T> |
There was a problem hiding this comment.
The allRows property has been added to the TableCellProps interface. Verify that this property is correctly passed and utilized in the component where necessary, and ensure it aligns with the intended functionality described in the pull request.
| @@ -40,7 +41,7 @@ const TableCell: <T extends { [propertyName: string]: any }>( | |||
| case 'action': | |||
| case 'element': | |||
| case 'numberElement': | |||
| data = props.renderer?.(props.data) | |||
| data = props.renderer?.(props.data, props.allRows) | |||
There was a problem hiding this comment.
The renderer function now receives an additional argument props.allRows. Ensure that the renderer function is designed to handle this new parameter appropriately. If renderer is a third-party or external function, verify its compatibility with this change.
| @@ -50,6 +51,7 @@ export const TableRow: <T extends { [propertyName: string]: any }>( | |||
| {...col} | |||
| data={props.data} | |||
| index={props.index} | |||
| allRows={props.allRows} | |||
There was a problem hiding this comment.
The addition of the allRows prop to the component should be verified for its necessity. Ensure that this prop is used within the component logic and that it aligns with the purpose of the pull request, which is to hide the action button when an invite is in progress. If allRows is not directly related to this functionality, consider removing it or clarifying its role.
...src/pages/copilot-opportunity-details/tabs/copilot-applications/CopilotApplicationAction.tsx
Show resolved
Hide resolved
...src/pages/copilot-opportunity-details/tabs/copilot-applications/CopilotApplicationAction.tsx
Show resolved
Hide resolved
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-580
What's in this PR?