-
Notifications
You must be signed in to change notification settings - Fork 0
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
Edit existing senior #121
Edit existing senior #121
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -54,7 +54,7 @@ const DisplaySenior = (props: DisplayProps) => { | |||
{/* @TODO - Firstname + lastname */} | |||
<h1 className="text-4xl font-bold text-[#000022]">{`${senior.firstname} ${senior.lastname}`}</h1> | |||
<p>{senior.description}</p> | |||
<Assigment editable={editable} senior={senior} /> | |||
<Assignment editable={editable} senior={senior} /> |
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.
Good catch :)
b162993
to
4b1a08a
Compare
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.
Changes look great overall, thank you for your work! I went ahead and made some small fixes:
- Rebase your branch onto latest main
- Move the state management logic from
patchAddSenior
tohandlePopup
as either flow ends up with the user clicking the confirming button, which triggershandlePopup
.
Other than that, I have some nits in the comment section that you may look into (if you have time). Otherwise, there's some styling issues we can fix (if you have time):
- I think the modal should be fixed height and scrolls as you assign / remove students for a given senior. Maybe we can have the student tags as a horizontal scroll?
- The current form implementation does not have any validation / user feedback. This might be enough work that it should be a ticket on its own. What do you think?
src/components/AddSenior.tsx
Outdated
location: "", | ||
description: "", | ||
}; | ||
}, []); |
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.
Should this be a constant outside the component?
src/components/SeniorView.tsx
Outdated
@@ -33,13 +35,48 @@ export const SeniorView = ({ seniors, students }: SeniorViewProps) => { | |||
/> | |||
} | |||
elements={seniorsState ? seniorsState : []} |
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.
seniorState
is defined as an array, so the ternary operator is not necessary.
fetch(`/api/senior/${senior.id}`, { | ||
method: "DELETE", | ||
}).then(() => { | ||
window.location.reload(); |
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.
Maybe we can filter the id from senior state?
src/components/SeniorView.tsx
Outdated
onClick: (e) => { | ||
e.stopPropagation(); | ||
e.preventDefault(); | ||
if (!setSeniorPatch || !setShowAddSeniorPopUp) { |
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.
When does this statement evaluate to true?
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.
Girlie pop, I just copied and pasted this mostly
Description
Make edit senior pop-up be initialized with the data we have of the senior that needs to be edited.
Issues
Resolve #82
Test
Follow the link below and change as needed. You need to be a chapter leader to edit it.
http://localhost:3000/private/[your userID]/chapter-leader/seniors
Possible Downsides
I did not test the delete functionality from the pop-up of the seniors.