Skip to content
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

Open
wants to merge 27 commits into
base: develop
Choose a base branch
from

Conversation

yushinj
Copy link
Contributor

@yushinj yushinj commented Jun 5, 2024

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:

train_trancsript

transcript1

@Luke-W-Hart Luke-W-Hart added the s24 Summer Practicum 2024 label Jun 5, 2024
@yushinj yushinj self-assigned this Jun 6, 2024
Comment on lines 36 to 49
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 '';
}
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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:

  1. I'm pretty sure your variable prev_job_title as scoped variable can be finicky. @EjPlatzer probably has an opinion on this
  2. there is some logic cleanup you can employ

@yushinj yushinj requested review from EjPlatzer and amos-cha and removed request for jsenning, russtuck and EjPlatzer June 11, 2024 13:36
Copy link
Contributor

@amos-cha amos-cha left a 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

src/services/transcript.ts Outdated Show resolved Hide resolved
src/services/transcript.ts Outdated Show resolved Hide resolved
src/services/transcript.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@amos-cha amos-cha left a 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


// sorting the same job title by end date
groupedMembershipHistory.experiences.sort((a, b) =>
getJobTitle(a).localeCompare(getJobTitle(b)) != 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
getJobTitle(a).localeCompare(getJobTitle(b)) != 0
getJobTitle(a).localeCompare(getJobTitle(b)) !== 0

@@ -32,6 +33,7 @@ const CoCurricularTranscript = () => {
return;
}
setLoading(true);
setPreviousTitles([]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
setPreviousTitles([]);
setPreviousTitles([]);

make a comment about this bugging out BECAUSE of the default transcript item. or else this looks confusing

amos-cha
amos-cha previously approved these changes Jun 13, 2024
Copy link
Contributor

@amos-cha amos-cha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@EjPlatzer EjPlatzer left a 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.

src/services/transcript.ts Outdated Show resolved Hide resolved
src/services/transcript.ts Outdated Show resolved Hide resolved
src/services/transcript.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@EjPlatzer EjPlatzer left a 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.

Comment on lines 34 to 54
.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;
}
}

Copy link
Contributor

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:

Suggested change
.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;
}

Comment on lines 14 to 19
const experienceTranscript =
jobTitles === ''
? styles.experience_transcript_activities_empty_titles
: styles.experience_transcript_activities;
return (
<div className={experienceTranscript}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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}>

Comment on lines 46 to 60
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 '';
};

Copy link
Contributor

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);

Copy link
Contributor

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>
)

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Comment on lines 140 to 143
//console.log('jobs');
//console.log(jobs);
//console.log('groupedMembershipHistory');
//console.log(groupedMembershipHistory);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//console.log('jobs');
//console.log(jobs);
//console.log('groupedMembershipHistory');
//console.log(groupedMembershipHistory);

Remove unused consolelogs

Comment on lines +46 to +49
if (Job_Title === '') {
return '';
}
return Job_Title === '' ? Job_Title : `${Job_Department_Name}, ${Job_Title}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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?

return getLatestEndDate(b)! - getLatestEndDate(a)!;
});

console.log(GroupByTitle);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
console.log(GroupByTitle);

remove console logs

: getExperienceEndDate(b) - getExperienceEndDate(a);
});

//groupedMembershipHistory.experiences = jobs;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
//groupedMembershipHistory.experiences = jobs;

remove unused code

package.json Outdated Show resolved Hide resolved
Co-authored-by: Evan Platzer <[email protected]>
Copy link
Member

@russtuck russtuck left a 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 = {
Copy link
Member

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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Job_Expected_Date?: string;
Job_Expected_End_Date?: string;

Keep meaning of this field.

Comment on lines +133 to +151
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];
}
Copy link
Member

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)!;
});
Copy link
Member

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';
Copy link
Member

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) => {
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s24 Summer Practicum 2024
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Repeated Information and Improve Legibility of Transcript
5 participants