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

Shows first semester GPA during second semester #126

Merged
merged 4 commits into from
Mar 2, 2020
Merged

Conversation

Suhas-13
Copy link
Member

@Suhas-13 Suhas-13 commented Mar 1, 2020

Closes #70

Uses fetch API to get grade history page and display first semester GPA during second semester.

@gary-kim gary-kim added the feature New feature addition label Mar 1, 2020
@gary-kim gary-kim added this to the Open Beta 0.19.0 milestone Mar 1, 2020
@Suhas-13
Copy link
Member Author

Suhas-13 commented Mar 1, 2020

Can this be merged or do you want me to wait for #112 and use the classes from that instead?

@gary-kim
Copy link
Member

gary-kim commented Mar 1, 2020

I've actually just been waiting for a review for that PR. Would you like to try reviewing it?

EDIT: You know what, @insertcustomname, how would you rather do this? Should we just merge this one first? I can refactor to use the classes here as well in #112 so would you rather merge this one first?

Copy link
Member

@gary-kim gary-kim left a comment

Choose a reason for hiding this comment

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

Code itself looks good but you seem to have some .DS_Store files in the commit as well as a merge from master. Could you rebase your branch on top of current master and get rid of the .DS_Store files?
You're also welcome to put a copyright header in the file.
Thank you!

EDIT: Also, I'm worried about a race condition in which the request for the 1st semester grades finishes first and the semester 1 GPA is below the semester 2 GPA. You think we could move that to after the semester 2 GPA has been added to the page?

Copy link
Member

@gary-kim gary-kim left a comment

Choose a reason for hiding this comment

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

Just to prevent accidental merge. See above :)

@Suhas-13
Copy link
Member Author

Suhas-13 commented Mar 2, 2020

I fixed the race condition, I tried adding the DS_store to the git ignore but not sure if it worked.

@gary-kim
Copy link
Member

gary-kim commented Mar 2, 2020

.DS_Store should probably be part of your global .gitignore but I'm okay with having it in the repo .gitignore if it means dealing with less Mac users accidentally committing the file. 😄
Also, you need to rebase your branch on top of the current master and remove the commits with dependency bumps. Actually, should I do it for you?

@Suhas-13
Copy link
Member Author

Suhas-13 commented Mar 2, 2020

Yeah that would be really helpful, thanks!

Suhas Hariharan added 4 commits March 2, 2020 20:14
Signed-off-by: Suhas Hariharan <hariharan774531@sas.edu.sg>
Signed-off-by: Gary Kim <gary@garykim.dev>
… of text parsing

Signed-off-by: Suhas Hariharan <hariharan774531@sas.edu.sg>
Signed-off-by: Suhas Hariharan <hariharan774531@sas.edu.sg>
Signed-off-by: Gary Kim <gary@garykim.dev>
Signed-off-by: Suhas Hariharan <hariharan774531@sas.edu.sg>
@gary-kim
Copy link
Member

gary-kim commented Mar 2, 2020

@insertcustomname What do you think?

@Suhas-13
Copy link
Member Author

Suhas-13 commented Mar 2, 2020

Great thanks, can we merge it?

Copy link
Member

@gary-kim gary-kim left a comment

Choose a reason for hiding this comment

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

LGTM

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature addition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show 1st Semester GPA
2 participants