Skip to content

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

Merged
merged 5 commits into from
Apr 25, 2025
Merged

PM-590 Add start date #1047

merged 5 commits into from
Apr 25, 2025

Conversation

himaniraghav3
Copy link
Collaborator

Related JIRA Ticket:

https://topcoder.atlassian.net/browse/PM-590

What's in this PR?

Adds start date to the details page:

image

<div className={styles.infoText}>
<span className={styles.infoHeading}>Start Date</span>
<span className={styles.infoValue}>
{moment(opportunity?.startDate)

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

image
@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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

image
@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) => (

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) => (

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] ?? '',

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;

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

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

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 && (

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])

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;

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;

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.

Copy link
Collaborator Author

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;

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;

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.

@himaniraghav3 himaniraghav3 merged commit d9d34af into dev Apr 25, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants