-
Notifications
You must be signed in to change notification settings - Fork 15
fix(PM-1373, PM-1375, PM-1376, PM-1383, PM-1380, PM-1378): demo feedbacks implementation #1116
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
Conversation
to providing a seamless and efficient process to ensure a | ||
great experience for all copilots. We will review your application | ||
within short time.` | ||
? `Thank you for taking the time to apply for this exciting opportunity. We truly value your interest and effort. Your application will be reviewed promptly.` |
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 a more formal tone by replacing 'promptly' with 'in a timely manner' to maintain consistency with the rest of the message.
ensure that you have carefully reviewed the project requirements and | ||
are committed to meeting them.` | ||
are committed to meeting them. Please write below the reason(s) why you believe you're a good fit for this project (e.g., previous experience, availability, etc.).` |
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 phrase 'Please write below the reason(s)' could be more concise. Consider rephrasing to 'Please provide the reason(s)'.
} | ||
</div> | ||
{ | ||
!success && ( | ||
<InputTextarea | ||
name='Notes' | ||
name='Reason' |
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 input field name has been changed from 'Notes' to 'Reason'. Ensure that this change is reflected consistently throughout the codebase, including any form handling logic or validation that may rely on the field name.
@@ -28,6 +28,7 @@ import { textFormatDateLocaleShortString } from '~/libs/shared' | |||
|
|||
import { CopilotApplication } from '../../models/CopilotApplication' | |||
import { | |||
cancelCopilotOpportunity, |
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 function cancelCopilotOpportunity
is imported but not used in this file. Consider removing it if it's not needed.
@@ -119,11 +120,25 @@ const CopilotOpportunityDetails: FC<{}> = () => { | |||
) | |||
} | |||
|
|||
async function cancelCopilotOpportunityHandler() { |
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 the cancelCopilotOpportunityHandler
function to manage potential failures from the cancelCopilotOpportunity
API call.
async function cancelCopilotOpportunityHandler() { | ||
if (opportunityId) { | ||
await cancelCopilotOpportunity(opportunityId) | ||
mutate(`${copilotBaseUrl}/copilots/opportunity/${opportunityId}/applications`) |
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 mutate
function is called twice with similar URLs. Consider extracting the base URL and opportunity ID into a variable to avoid repetition and improve readability.
@@ -136,6 +151,7 @@ const CopilotOpportunityDetails: FC<{}> = () => { | |||
&& opportunity?.status === 'active' | |||
&& opportunity?.canApplyAsCopilot ? applyCopilotOpportunityButton : undefined | |||
} | |||
secondaryButtonConfig={opportunity?.status === 'active' && isAdminOrPM ? cancelCopilotOpportunityButton : undefined} |
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 checking if isAdminOrPM
is defined and has a valid value before using it in the conditional expression to avoid potential runtime errors.
@@ -1,5 +1,9 @@ | |||
@import '@libs/ui/styles/includes'; | |||
|
|||
.wrapper { | |||
min-height: 800px; |
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 a more flexible unit for min-height
, such as vh
or %
, to ensure better responsiveness across different screen sizes.
@@ -15,7 +15,7 @@ const CopilotApplicationAction = ( | |||
): JSX.Element => { | |||
const { opportunityId }: {opportunityId?: string} = useParams<{ opportunityId?: string }>() | |||
const isInvited = useMemo( | |||
() => allCopilotApplications.findIndex(item => item.status === CopilotApplicationStatus.INVITED) > -1, | |||
() => allCopilotApplications && allCopilotApplications.findIndex(item => item.status === CopilotApplicationStatus.INVITED) > -1, |
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 optional chaining (?.
) for safer access to allCopilotApplications
. This can prevent potential runtime errors if allCopilotApplications
is null
or undefined
. For example: allCopilotApplications?.findIndex(...)
.
{skill.name} | ||
</div> | ||
))} | ||
{props.opportunity?.skills.map(item => item.name).join(",")} |
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 current implementation concatenates skill names into a single string separated by commas. This change removes the previous styling applied to each skill using styles.skillPill
. If the styling is still required, consider mapping over the skills to create individual styled elements instead of joining them into a single string.
<span | ||
className={styles.infoValue} | ||
> | ||
{opportunity?.type && getOpportunityType(opportunity?.type)} |
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 function getOpportunityType
is used here to transform opportunity?.type
. Ensure that this function handles all possible values of opportunity?.type
and returns a meaningful result or a default value if the type is not recognized.
@@ -47,6 +48,7 @@ import { | |||
import { OpportunityDetails } from './tabs/opportunity-details' | |||
import { CopilotApplications } from './tabs/copilot-applications' | |||
import styles from './styles.module.scss' | |||
import { toast } from 'react-toastify' |
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.
It seems that toast
from react-toastify
is imported but not used in this file. Consider removing the import if it is not needed to keep the code clean and avoid unnecessary dependencies.
@@ -104,7 +106,7 @@ const CopilotOpportunityDetails: FC<{}> = () => { | |||
|
|||
const onApplied: () => void = useCallback(() => { | |||
mutate(`${copilotBaseUrl}/copilots/opportunity/${opportunityId}/applications`) | |||
mutate(`${copilotBaseUrl}/copilots/opportunity/${opportunityId}`) | |||
mutate(`${copilotBaseUrl}/copilot/opportunity/${opportunityId}`) |
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 URL path has been changed from 'copilots/opportunity' to 'copilot/opportunity'. Please ensure that this change is intentional and that the new path is correct and consistent with the API endpoint.
if (opportunityId) { | ||
await cancelCopilotOpportunity(opportunityId) | ||
mutate(`${copilotBaseUrl}/copilots/opportunity/${opportunityId}/applications`) | ||
mutate(`${copilotBaseUrl}/copilot/opportunity/${opportunityId}`) |
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 URL path in the mutate
function call has been changed from copilots
to copilot
. Ensure that this change is intentional and that the new path is correct, as it might affect the data fetching logic.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-1373
https://topcoder.atlassian.net/browse/PM-1375
https://topcoder.atlassian.net/browse/PM-1376
https://topcoder.atlassian.net/browse/PM-1383
https://topcoder.atlassian.net/browse/PM-1380
https://topcoder.atlassian.net/browse/PM-1378
What's in this PR?