reduce redundancy in transcript #2280
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,19 +1,27 @@ | ||||||||||||||||||||||
| import { format } from 'date-fns'; | ||||||||||||||||||||||
| import { StudentEmployment } from 'services/transcript'; | ||||||||||||||||||||||
| import styles from './Experience.module.css'; | ||||||||||||||||||||||
| import { Dispatch, SetStateAction } from 'react'; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| type Props = { | ||||||||||||||||||||||
| Experience: StudentEmployment; | ||||||||||||||||||||||
| previousTitles: string[]; | ||||||||||||||||||||||
| setPreviousTitles: Dispatch<SetStateAction<string[]>>; | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const Experience = ({ Experience, previousTitles, setPreviousTitles }: Props) => { | ||||||||||||||||||||||
| const jobTitles = newJobTitle(Experience, previousTitles, setPreviousTitles); | ||||||||||||||||||||||
| const experienceTranscript = | ||||||||||||||||||||||
| jobTitles === '' | ||||||||||||||||||||||
| ? styles.experience_transcript_activities_empty_titles | ||||||||||||||||||||||
| : styles.experience_transcript_activities; | ||||||||||||||||||||||
| return ( | ||||||||||||||||||||||
| <div className={experienceTranscript}> | ||||||||||||||||||||||
|
||||||||||||||||||||||
| const experienceTranscript = | |
| jobTitles === '' | |
| ? styles.experience_transcript_activities_empty_titles | |
| : styles.experience_transcript_activities; | |
| return ( | |
| <div className={experienceTranscript}> | |
| return ( | |
| <div className={jobTitles === '' | |
| ? `${styles.experience_transcript_activities} ${styles.empty_title}` | |
| : styles.experience_transcript_activities}> |
Outdated
There was a problem hiding this comment.
There is another way to do this, which in my opinion is both easier and more maintainable.
Basically, you can use Object.groupBy to group the experiences by Job_Title, and display the title once per group of jobs with the same title.
The grouping is as simple as:
const grouped_experiences = Object.groupBy(experiences, (experience) => experience.Job_Title);There was a problem hiding this comment.
To give a more complete example, here's a simplified experiences array:
const jobs = [
{title: 'Dishwasher', start: new Date('2016-08-20'), end: new Date('2017-05-15')},
{title: 'RA', start: new Date('2017-08-01'), end: new Date('2018-05-25')},
{title: 'RA', start: new Date('2018-08-02'), end: new Date('2019-05-10')}
];and we can group it like so:
const jobsByTitle = Object.groupBy(jobs, job => job.title);we can convert that object to an array of objects, where each object in the array has a Job_Title property which is the title of all the jobs in that group, and a jobs property which is the array containing all the jobs in that group:
const jobGroups = Object.entries(jobsByTitle).map(([title, jobs]) => ({Job_Title: title, jobs}))then we can render those job groups:
const Experience = ({Job_Title, jobs}) => {
return (
<div className={styles.experience_transcript_activities}>
<div className={styles.organization_role}>{Job_Title}</div>
{jobs.map((job) => (
<div>{formatDuration(job)}</div>
)}
</div>
)
There was a problem hiding this comment.
Thank you for your suggestion. Can you explain why using group is easier and more maintainable? This code takes an array of information one by one from the ordered list. If I use group, it need an extra function to extract data from the group and a function that is similar to the upper code to display one title per group and to ignore job titles in the group, which is more complicate.
There was a problem hiding this comment.
Because your current code is using React's useState hook to keep track of whether a certain title has been seen yet or not, you have to update state (and thus re-render the entire transcript) once for each experience. If you instead group experiences by job title before passing the data to React (i.e. within services/transcript.ts), you will already have all the data calculated and won't have to re-render any extra times.
There was a problem hiding this comment.
Thank you for your answer! I tried to group experiences within services/transcript.ts (line 51-78), but the type of data will be changed from groupedMembershipHistory to array if I group by job title. Is there better way to group experience or improve the code without groupBy?
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -19,6 +19,7 @@ const SectionTitle: { [Key in keyof TranscriptItems]: string } = { | |||||
| }; | ||||||
|
|
||||||
| const CoCurricularTranscript = () => { | ||||||
| const [previousTitles, setPreviousTitles] = useState<string[]>([]); | ||||||
| const [loading, setLoading] = useState(true); | ||||||
| const [transcriptItems, setTranscriptItems] = useState<TranscriptItems | undefined>(); | ||||||
| const isAuthenticated = useIsAuthenticated(); | ||||||
|
|
@@ -32,6 +33,11 @@ const CoCurricularTranscript = () => { | |||||
| return; | ||||||
| } | ||||||
| setLoading(true); | ||||||
| /** | ||||||
| * When you return the transcript from other page, the job titles will be lost | ||||||
| * Fixing bug by setting PreviousTitles to empty array when the transcript is loaded | ||||||
| */ | ||||||
| setPreviousTitles([]); | ||||||
|
||||||
| setPreviousTitles([]); | |
| setPreviousTitles([]); |
make a comment about this bugging out BECAUSE of the default transcript item. or else this looks confusing
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.
Return this new GroupByTitle in groupedMembershipHistory, NOT the de-grouped version below. (Why do all the work to combine each job into one entry with multiple times, and then not use it?)