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

Creation of Edit Student Modal- Issue #314 #315

Merged
merged 5 commits into from
Apr 17, 2024
Merged

Conversation

katconnors
Copy link
Contributor

@katconnors katconnors commented Apr 15, 2024

Link to issue:
#314

Styling will be in a separate issue/PR

@katconnors katconnors changed the title Creation of Edit Student Module- Issue #314 Creation of Edit Student Modal- Issue #314 Apr 15, 2024
@KCCPMG
Copy link
Contributor

KCCPMG commented Apr 17, 2024

This looks like it works and my comments are pretty minor clean-up stuff. One thing I noticed (which is beyond the scope of this Issue/PR) is that you can edit a student even when there's no IEP, which is good, except that the edit form includes the fields for the IEP dates (which also don't render well when there's no dates to load). I don't consider it necessary for this PR, but it might be good to add a quick check and render those last two fields only on the condition that an IEP exists.

const ViewStudentPage = () => {
const [open, setOpen] = React.useState(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can just be useState instead of React.useState

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed!

@@ -24,10 +49,11 @@ const ViewStudentPage = () => {
const router = useRouter();
const { student_id } = router.query;

const VIEW_STATES = { MAIN: 0, EDIT: 1 };
const VIEW_STATES = { MAIN: 0 };
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like the VIEW_STATES are no longer necessary and they don't change anymore...could we get rid of this altogether?

Copy link
Contributor Author

@katconnors katconnors Apr 17, 2024

Choose a reason for hiding this comment

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

Removed this, along with other references to view state

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed the viewState and setViewState on line 46, which I think are a part of this and also outdated. I didn't even notice this before and I'll still approve

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Removed this as well.

@katconnors katconnors requested a review from KCCPMG April 17, 2024 01:20
@katconnors
Copy link
Contributor Author

This looks like it works and my comments are pretty minor clean-up stuff. One thing I noticed (which is beyond the scope of this Issue/PR) is that you can edit a student even when there's no IEP, which is good, except that the edit form includes the fields for the IEP dates (which also don't render well when there's no dates to load). I don't consider it necessary for this PR, but it might be good to add a quick check and render those last two fields only on the condition that an IEP exists.

I also removed some other references to view state, so will request re-review on your end, when you have time! Thanks Connor!

@katconnors katconnors merged commit 73e11ca into main Apr 17, 2024
3 checks passed
@katconnors katconnors deleted the kconnors-module-test branch April 17, 2024 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants