feat(PM-3563): added other industry to work experience form #1421
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| const ModalAddWork: FC<ModalAddWorkProps> = (props: ModalAddWorkProps) => { | ||
| const [workInfo, setWorkInfo] = useState(emptyWorkInfo()) | ||
| const [formErrors, setFormErrors] = useState<any>({ |
There was a problem hiding this comment.
[maintainability]
Consider using a more specific type instead of any for formErrors to improve type safety and maintainability.
| errorTmp.endDate = 'Required' | ||
| } | ||
|
|
||
| if (workInfo.industry === 'Other' && !workInfo.otherIndustry?.trim()) { |
There was a problem hiding this comment.
[❗❗ correctness]
The check for workInfo.otherIndustry?.trim() assumes otherIndustry is a string. Ensure workInfo.otherIndustry is always initialized as a string to avoid potential runtime errors.
| setWorkInfo({ | ||
| ...workInfo, | ||
| industry: event.target.value, | ||
| otherIndustry: event.target.value === 'Other' ? workInfo.otherIndustry : undefined, |
There was a problem hiding this comment.
[correctness]
When setting otherIndustry to undefined, ensure that this does not unintentionally affect other parts of the application that may rely on otherIndustry being a string.
| name='otherIndustry' | ||
| label='Please specify your industry *' | ||
| value={workInfo.otherIndustry || ''} | ||
| onChange={function onChange(event: any) { |
There was a problem hiding this comment.
[💡 maintainability]
Consider using a more specific type instead of any for the event parameter in the onChange function to improve type safety.
| case 'industry': | ||
| value = event.target.value | ||
| if (value !== 'Other') { | ||
| oldFormValues.otherIndustry = undefined |
There was a problem hiding this comment.
[💡 readability]
Consider using delete oldFormValues.otherIndustry instead of setting it to undefined for better clarity and to avoid potential issues with object property checks.
| } | ||
| } | ||
|
|
||
| if (formValues.industry === 'Other' && !trim(formValues.otherIndustry as string)) { |
There was a problem hiding this comment.
[correctness]
The check for formValues.industry === 'Other' and !trim(formValues.otherIndustry as string) is correct, but ensure that formValues.otherIndustry is always a string before trimming to avoid runtime errors.
| position && industry | ||
| ? `, ${getIndustryOptionLabel(industry)}` | ||
| : (industry ? getIndustryOptionLabel(industry) : '') | ||
| ? `, ${industry === 'Other' |
There was a problem hiding this comment.
[readability]
The nested ternary operators make this logic difficult to read and understand. Consider refactoring this into a separate function to improve readability and maintainability.
|
|
||
| if (keys.includes(v)) { | ||
| return IndustryEnumToLabel[v] | ||
| return v |
There was a problem hiding this comment.
[❗❗ correctness]
The change from IndustryEnumToLabel[v] to v in getIndustryOptionValue might lead to incorrect behavior if v is expected to be a key in IndustryEnumToLabel. This could result in returning the key itself instead of its corresponding label. Verify if this change aligns with the intended logic.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-3563
What's in this PR?