-
Notifications
You must be signed in to change notification settings - Fork 10
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
reduce redundancy in transcript #2280
base: develop
Are you sure you want to change the base?
Conversation
let prev_job_title = ''; | ||
|
||
const newJobTitle = ({ Job_Department_Name, Job_Title }: StudentEmployment) => { | ||
if (Job_Title != prev_job_title) { | ||
prev_job_title = Job_Title; | ||
if (Job_Department_Name == Job_Title.split(':')[0]) { | ||
return Job_Title; | ||
} else { | ||
return Job_Department_Name + ', ' + Job_Title; | ||
} | ||
} else { | ||
return ''; | ||
} | ||
}; |
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.
let prev_job_title = ''; | |
const newJobTitle = ({ Job_Department_Name, Job_Title }: StudentEmployment) => { | |
if (Job_Title != prev_job_title) { | |
prev_job_title = Job_Title; | |
if (Job_Department_Name == Job_Title.split(':')[0]) { | |
return Job_Title; | |
} else { | |
return Job_Department_Name + ', ' + Job_Title; | |
} | |
} else { | |
return ''; | |
} | |
}; | |
let prev_job_title = ''; | |
const newJobTitle = ({ Job_Department_Name, Job_Title }: StudentEmployment) => { | |
if (Job_Title != prev_job_title) { | |
prev_job_title = Job_Title; | |
return Job_Department_Name === Job_Title.split(':')[0] | |
? Job_Title | |
: `${Job_Department_Name}, ${Job_Title}`; | |
} | |
return ''; | |
}; |
Couple things:
- I'm pretty sure your variable
prev_job_title
as scoped variable can be finicky. @EjPlatzer probably has an opinion on this - there is some logic cleanup you can employ
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.
almost there, just a bit of cleanup
Co-authored-by: Amos Cha <78386128+amos-cha@users.noreply.github.com>
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.
2 more things i just saw
src/services/transcript.ts
Outdated
|
||
// sorting the same job title by end date | ||
groupedMembershipHistory.experiences.sort((a, b) => | ||
getJobTitle(a).localeCompare(getJobTitle(b)) != 0 |
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.
getJobTitle(a).localeCompare(getJobTitle(b)) != 0 | |
getJobTitle(a).localeCompare(getJobTitle(b)) !== 0 |
@@ -32,6 +33,7 @@ const CoCurricularTranscript = () => { | |||
return; | |||
} | |||
setLoading(true); | |||
setPreviousTitles([]); |
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.
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.
lgtm
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.
Thanks for working on this! I haven't gotten the chance to review the rest of the code yet, but I have a couple of comments on the parts in services/transcript.ts
.
Co-authored-by: Evan Platzer <evan.platzer@gordon.edu>
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.
I think this is great work, thank you for improving the transcript.
The current way you're handling repeated jobs does seem to work, but it's more complicated than it needs to be. I commented below with an alternate approach where you group the experiences by their job_title so that you can display the job title only once per group of jobs with the same title.
.experience_transcript_activities_empty_titles { | ||
display: grid; | ||
grid-template-columns: 5% 70% 25%; | ||
grid-template-rows: auto auto; | ||
justify-items: stretch; | ||
position: relative; | ||
top: -20px; | ||
|
||
.organization_role { | ||
grid-column: 2; | ||
grid-row: 1; | ||
text-align: left; | ||
} | ||
|
||
.date { | ||
grid-column: 3; | ||
grid-row: 1; | ||
text-align: left; | ||
} | ||
} | ||
|
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's not ideal to duplicate the whole styles, just to add the position: relative
and top: -20px
. It would be better to keep those in a separate class and simply apply both classes when needed:
.experience_transcript_activities_empty_titles { | |
display: grid; | |
grid-template-columns: 5% 70% 25%; | |
grid-template-rows: auto auto; | |
justify-items: stretch; | |
position: relative; | |
top: -20px; | |
.organization_role { | |
grid-column: 2; | |
grid-row: 1; | |
text-align: left; | |
} | |
.date { | |
grid-column: 3; | |
grid-row: 1; | |
text-align: left; | |
} | |
} | |
.empty_title { | |
position: relative; | |
top: -20px; | |
} | |
const experienceTranscript = | ||
jobTitles === '' | ||
? styles.experience_transcript_activities_empty_titles | ||
: styles.experience_transcript_activities; | ||
return ( | ||
<div className={experienceTranscript}> |
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.
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}> |
const newJobTitle = ( | ||
{ Job_Department_Name, Job_Title }: StudentEmployment, | ||
previousTitles: string[], | ||
setPreviousTitles: Dispatch<SetStateAction<string[]>>, | ||
) => { | ||
if (!previousTitles.includes(Job_Title)) { | ||
previousTitles.push(Job_Title); | ||
setPreviousTitles(previousTitles); | ||
return Job_Department_Name === Job_Title.split(':')[0] | ||
? Job_Title | ||
: `${Job_Department_Name}, ${Job_Title}`; | ||
} | ||
return ''; | ||
}; | ||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
src/services/transcript.ts
Outdated
//console.log('jobs'); | ||
//console.log(jobs); | ||
//console.log('groupedMembershipHistory'); | ||
//console.log(groupedMembershipHistory); |
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.
//console.log('jobs'); | |
//console.log(jobs); | |
//console.log('groupedMembershipHistory'); | |
//console.log(groupedMembershipHistory); |
Remove unused consolelogs
if (Job_Title === '') { | ||
return ''; | ||
} | ||
return Job_Title === '' ? Job_Title : `${Job_Department_Name}, ${Job_Title}`; |
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.
if (Job_Title === '') { | |
return ''; | |
} | |
return Job_Title === '' ? Job_Title : `${Job_Department_Name}, ${Job_Title}`; | |
if (Job_Title === '') { | |
return ''; | |
} | |
return Job_Title === '' ? Job_Title : `${Job_Department_Name}, ${Job_Title}`; |
that top part and the first half of the ternary are logically identical. Is there a reason both are included?
src/services/transcript.ts
Outdated
return getLatestEndDate(b)! - getLatestEndDate(a)!; | ||
}); | ||
|
||
console.log(GroupByTitle); |
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.
console.log(GroupByTitle); |
remove console logs
src/services/transcript.ts
Outdated
: getExperienceEndDate(b) - getExperienceEndDate(a); | ||
}); | ||
|
||
//groupedMembershipHistory.experiences = jobs; |
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.
//groupedMembershipHistory.experiences = jobs; |
remove unused code
Co-authored-by: Evan Platzer <evan.platzer@gordon.edu>
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.
Please split this into 2 pull requests, so the dining points pie chart won't be in here with jobs.
latestDate?: string; | ||
}; | ||
|
||
export type NewStudentEmployment = { |
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.
This needs to become StudentEmployment, and the old one above should be deleted.
export type session = { | ||
Job_Start_Date?: string; | ||
Job_End_Date?: string; | ||
Job_Expected_Date?: string; |
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.
Job_Expected_Date?: string; | |
Job_Expected_End_Date?: string; |
Keep meaning of this field.
let test = []; | ||
for (let i = 0; i < GroupByTitle.length; i++) { | ||
let tempJob = GroupByTitle?.[i].job; | ||
let numJobs = tempJob!.length; | ||
for (let j = 0; j < numJobs; j++) { | ||
let tempVal = GroupByTitle[i]; | ||
let value = tempVal!.job?.[j]; | ||
if (j > 0) { | ||
value!.Job_Title = ''; | ||
} | ||
if (value) { | ||
test.push(value); | ||
} | ||
} | ||
} | ||
|
||
for (let i = 0; i < test.length; i++) { | ||
jobs[i] = test[i]; | ||
} |
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.
I think this should be deleted.
// Sorting the grouped job by the latest end date | ||
GroupByTitle.sort((a, b) => { | ||
return getLatestEndDate(b)! - getLatestEndDate(a)!; | ||
}); |
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?)
@@ -11,7 +11,7 @@ import { | |||
import EditIcon from '@mui/icons-material/Edit'; | |||
import GordonLoader from 'components/Loader'; | |||
import { useEffect, useState } from 'react'; | |||
import { Doughnut } from 'react-chartjs-2'; | |||
import { Pie } from 'react-chartjs-2'; |
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.
Why is this unrelated change in this pull request. Please put it in a different branch and make a pull request just for it. Then it won't need to wait for the other parts you are still working on.
<div className={styles.experience_transcript_activities}> | ||
<div className={styles.organization_role}> | ||
{Experience.Job_Department_Name}, {Experience.Job_Title} | ||
const Experience = ({ Experience }: Props) => { |
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.
I think this could be simpler if you passed in the grouped jobs.
…/gordon-cs/gordon-360-ui into s24-remove-redundancy-transcript
Some departments have the same string on Job_Title and Job_Department_Name. Also, transcript repeats the Job_title for each session. So, remove redundant information from transcript.
Example: