-
Notifications
You must be signed in to change notification settings - Fork 13
PM-590 Add start date #1047
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PM-590 Add start date #1047
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: