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

HY - PSCourse dropdown #30

Merged
merged 3 commits into from Nov 27, 2022
Merged

HY - PSCourse dropdown #30

merged 3 commits into from Nov 27, 2022

Conversation

HansonYu23
Copy link
Contributor

Replaced Personal Schedule ID field in PSCourse create page with Personal schedule dropdown menu. Clicking "Create" when both a valid enrollment code is entered and a Personal schedule is selected calls api/courses/post for the entered values. Modified associated tests to account for changes.

Screenshot (3)

Resolves #11

@HansonYu23 HansonYu23 self-assigned this Nov 27, 2022
@HansonYu23 HansonYu23 added the f22-7pm-2 f22-7pm-2 label Nov 27, 2022
@HansonYu23 HansonYu23 changed the title Hy PSCourse dropdown HY PSCourse dropdown Nov 27, 2022
@HansonYu23 HansonYu23 changed the title HY PSCourse dropdown HY - PSCourse dropdown Nov 27, 2022
Copy link

@andysglez andysglez left a comment

Choose a reason for hiding this comment

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

LGTM!

@pconrad pconrad added the STAFF: ready for review green on CI, has peer CR, no other obvious issues label Nov 27, 2022
@pconrad pconrad temporarily deployed to f22-7pm-2-courses November 27, 2022 19:38 Inactive
Copy link
Contributor

@pconrad pconrad left a comment

Choose a reason for hiding this comment

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

LGTM mostly... I did have one concern. I'm still going to approve and merge, but I encourage you to take a look at this.

expect(axiosMock.history.post[0].params).toEqual(
{
"psId": "13",
"psId": "",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is suspicious... why is this an empty string here?

I'll let it slide, but I think this might come back to bite you later.
so I want to encourage you to take a look

@pconrad pconrad merged commit 76cae39 into main Nov 27, 2022
@pconrad pconrad added 10 10 pts and removed STAFF: ready for review green on CI, has peer CR, no other obvious issues labels Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10 10 pts f22-7pm-2 f22-7pm-2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace Personal Schedule ID row in PSCourses create page with dropdown menu
3 participants