PM-590 Add start date #1047
PM-590 Add start date #1047
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/apps/copilots/src/pages/copilot-opportunity-details/index.tsx
Outdated
Show resolved
Hide resolved
| <div className={styles.infoText}> | ||
| <span className={styles.infoHeading}>Start Date</span> | ||
| <span className={styles.infoValue}> | ||
| {moment(opportunity?.startDate) |
There was a problem hiding this comment.
Consider adding a fallback or default value for opportunity?.startDate in case it is undefined or null to prevent potential runtime errors.
There was a problem hiding this comment.
@himaniraghav3 checked this - looks like moment will intialize with current datetime in case of undefined startDate.
We fine with that?
| <div className={styles.infoText}> | ||
| <span className={styles.infoHeading}>Start Date</span> | ||
| <span className={styles.infoValue}> | ||
| {moment(opportunity?.startDate) |
There was a problem hiding this comment.
@himaniraghav3 checked this - looks like moment will intialize with current datetime in case of undefined startDate.
We fine with that?
|
|
||
| import styles from './styles.module.scss' | ||
|
|
||
| const tableColumns: TableColumn<CopilotOpportunity>[] = [ | ||
| { | ||
| label: 'Title', | ||
| propertyName: 'projectName', | ||
| type: 'text', | ||
| renderer: (copilotOpportunity: CopilotOpportunity) => ( |
There was a problem hiding this comment.
Suggestion
Consider using a more descriptive element or adding a class to the <div> for better styling and accessibility. This can help with future styling changes or accessibility improvements.
| @@ -49,7 +56,12 @@ const tableColumns: TableColumn<CopilotOpportunity>[] = [ | |||
| { | |||
| label: 'Type', | |||
| propertyName: 'type', | |||
| type: 'text', | |||
| renderer: (copilotOpportunity: CopilotOpportunity) => ( | |||
There was a problem hiding this comment.
Consider handling cases where copilotOpportunity.projectType might not have a corresponding label in ProjectTypeLabels. This will prevent potential runtime errors if an unexpected projectType value is encountered.
| const tableData = useMemo(() => opportunities, [opportunities]) | ||
| const tableData = useMemo(() => opportunities.map(opportunity => ({ | ||
| ...opportunity, | ||
| type: ProjectTypeLabels[opportunity.projectType] ?? '', |
There was a problem hiding this comment.
Consider providing a default value or handling the case where opportunity.projectType might not exist in ProjectTypeLabels to avoid potential issues with undefined values.
| @@ -3,6 +3,8 @@ | |||
| .skillsContainer { | |||
| display: flex; | |||
| flex-wrap: wrap; | |||
| overflow-x: auto; | |||
There was a problem hiding this comment.
Consider using overflow: hidden instead of overflow-y: hidden to ensure that any overflow in both directions is handled consistently, unless there is a specific reason to only hide vertical overflow.
| const col = props.columns.find(c => c.propertyName === fieldName) | ||
|
|
||
| // if sortable is false, we return | ||
| if (!col?.isSortable === false) return |
There was a problem hiding this comment.
The condition !col?.isSortable === false is confusing and may not work as intended. Consider simplifying it to col?.isSortable === false or !(col?.isSortable) depending on the intended logic.
| @@ -129,7 +134,7 @@ const Table: <T extends { [propertyName: string]: any }>(props: TableProps<T>) = | |||
|
|
|||
| const headerRow: Array<JSX.Element> = displayColumns | |||
| .map((col, index) => { | |||
| const isSortable: boolean = !!col.propertyName | |||
| const isSortable: boolean = !!col.propertyName && col.isSortable !== false | |||
There was a problem hiding this comment.
The condition col.isSortable !== false could be simplified to col.isSortable if isSortable is expected to be a boolean. This would make the code more concise and easier to read.
| @@ -151,7 +156,7 @@ const Table: <T extends { [propertyName: string]: any }>(props: TableProps<T>) = | |||
| </Tooltip> | |||
| </div> | |||
| )} | |||
| {!props.disableSorting && ( | |||
| {!props.disableSorting && isSortable && ( | |||
There was a problem hiding this comment.
The addition of isSortable changes the condition for rendering the TableSort component. Ensure that isSortable is properly defined and initialized in the component's logic to avoid potential runtime errors or unintended behavior.
| sort.direction, | ||
| )) | ||
| .sort((a: T, b: T) => { | ||
| const aDate = new Date(a[sort.fieldName]) |
There was a problem hiding this comment.
Consider adding error handling for invalid date strings. Using new Date() without validation can result in 'Invalid Date' objects, which may cause unexpected behavior.
| @@ -3,6 +3,8 @@ | |||
| .skillsContainer { | |||
| display: flex; | |||
| flex-wrap: wrap; | |||
| overflow: auto; | |||
There was a problem hiding this comment.
Combining overflow-x and overflow-y into overflow changes the behavior to allow both horizontal and vertical scrolling. Ensure this change is intentional and does not affect the layout negatively.
| @@ -3,6 +3,8 @@ | |||
| .skillsContainer { | |||
| display: flex; | |||
| flex-wrap: wrap; | |||
| overflow: auto; | |||
| max-width: 300px; | |||
There was a problem hiding this comment.
Adding max-width: 300px; restricts the container's width, which might cause layout issues if the content exceeds this width. Verify that this constraint is suitable for all screen sizes and content scenarios.
There was a problem hiding this comment.
This was needed to add a scroll to the skills column instead of the table, when the skill pills were too long
| @@ -3,6 +3,7 @@ | |||
| .skillsContainer { | |||
| display: flex; | |||
| flex-wrap: wrap; | |||
| overflow: auto; | |||
| gap: 8px; | |||
There was a problem hiding this comment.
The removal of max-width: 300px; might affect the layout if there was a reason to constrain the width previously. Please ensure that this change does not negatively impact the design or layout of the page.
| @@ -11,7 +12,7 @@ | |||
| color: #333; | |||
| padding: 4px 8px; | |||
| border-radius: 10px; | |||
| white-space: nowrap; | |||
| white-space: break-spaces; | |||
There was a problem hiding this comment.
Changing white-space from nowrap to break-spaces may affect the layout and readability of the text. Consider whether this change is necessary and test to ensure it doesn't negatively impact the design, especially in responsive scenarios.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-590
What's in this PR?
Adds start date to the details page: