-
Notifications
You must be signed in to change notification settings - Fork 21
feat(PM-3398): added description and associated skills field in Add/Update Work experience modal, work card in Profile Page #1398
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
...iles/src/member-profile/work-expirence/ModifyWorkExpirenceModal/ModifyWorkExpirenceModal.tsx
Outdated
Show resolved
Hide resolved
...iles/src/member-profile/work-expirence/ModifyWorkExpirenceModal/ModifyWorkExpirenceModal.tsx
Show resolved
Hide resolved
...iles/src/member-profile/work-expirence/ModifyWorkExpirenceModal/ModifyWorkExpirenceModal.tsx
Show resolved
Hide resolved
...iles/src/member-profile/work-expirence/ModifyWorkExpirenceModal/ModifyWorkExpirenceModal.tsx
Show resolved
Hide resolved
src/apps/profiles/src/member-profile/work-expirence/WorkExpirence.tsx
Outdated
Show resolved
Hide resolved
src/apps/profiles/src/member-profile/work-expirence/WorkExpirence.tsx
Outdated
Show resolved
Hide resolved
src/apps/profiles/src/member-profile/work-expirence/WorkExpirence.tsx
Outdated
Show resolved
Hide resolved
src/apps/profiles/src/member-profile/work-expirence/WorkExpirence.tsx
Outdated
Show resolved
Hide resolved
...s/profiles/src/member-profile/work-expirence/WorkExpirenceCard/WorkExpirenceCard.module.scss
Show resolved
Hide resolved
src/apps/profiles/src/member-profile/work-expirence/WorkExpirenceCard/WorkExpirenceCard.tsx
Show resolved
Hide resolved
src/apps/profiles/src/member-profile/work-expirence/WorkExpirenceCard/WorkExpirenceCard.tsx
Show resolved
Hide resolved
...iles/src/member-profile/work-expirence/ModifyWorkExpirenceModal/ModifyWorkExpirenceModal.tsx
Show resolved
Hide resolved
src/libs/shared/lib/services/standard-skills/standard-skills.service.ts
Outdated
Show resolved
Hide resolved
| @@ -1,7 +1,8 @@ | |||
| import { Dispatch, FC, SetStateAction, useEffect, useMemo, useState } from 'react' | |||
| import { Dispatch, FC, SetStateAction, useCallback, useEffect, useMemo, useState } from 'react' | |||
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.
[❗❗ correctness]
The useRef import has been removed but is still used in the code. Ensure that fetchedSkillIdsRef is no longer needed or re-import useRef if it is still required.
| const workExpirence: UserTrait[] | undefined | ||
| = useMemo(() => memberWorkExpirenceTraits?.[0]?.traits?.data, [memberWorkExpirenceTraits]) | ||
|
|
||
| // Collect all unique skill IDs from work experience entries |
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.
[performance]
The useMemo hook is used to collect skill IDs, which is appropriate for performance optimization. However, ensure that the dependencies are correctly set to avoid unnecessary recalculations.
| }, [workExpirence]) | ||
|
|
||
| // Fetch skills using SWR hook | ||
| const { data: fetchedSkills, error: skillsError } = useSkillsByIds( |
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.
[maintainability]
Consider handling the case where useSkillsByIds returns an error (skillsError). Currently, the error is only used to determine the loading state, but it might be beneficial to handle or log the error for better debugging and user feedback.
| return map | ||
| }, [fetchedSkills, allSkillIds]) | ||
|
|
||
| const areSkillsLoaded = useCallback((work: UserTrait): boolean => { |
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.
[performance]
The useCallback hook is used for areSkillsLoaded, which is good for preventing unnecessary re-creations of the function. Ensure that the dependencies are correctly set to avoid stale closures.
|
|
||
| return useSWR<UserSkill[], Error>( | ||
| swrKey, | ||
| () => fetchSkillsByIdsFetcher(skillIds!), |
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.
[❗❗ correctness]
Using the non-null assertion operator (!) on skillIds can lead to runtime errors if skillIds is undefined. Consider adding a check or default value to ensure safety.
| if (!skillIds || skillIds.length === 0) { | ||
| return null | ||
| } | ||
| return ['skills-by-ids', [...skillIds].sort().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.
[performance]
Sorting skillIds before joining them into a string for the SWR key ensures consistent key generation, but it might introduce unnecessary overhead if skillIds are already sorted. Consider checking if sorting is necessary to optimize performance.
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-3398
What's in this PR?
Screenshots