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

Senior form feedback #151

Merged
merged 6 commits into from
Apr 16, 2024
Merged

Senior form feedback #151

merged 6 commits into from
Apr 16, 2024

Conversation

cledi01
Copy link
Collaborator

@cledi01 cledi01 commented Apr 10, 2024

Description

Updated the add/update senior form pop-up to be compatible with the react-hook-form module. Also made the submit button only visible when fields have been edited.

Issues

#124

Test

To test, go to http://localhost:3000/private/chapter-leader/seniors

Make sure to make yourself a chapter-leader beforehand on the database manually to get to this page.

Possible Downsides

When testing, I didn't have google folder access to the chapter's folder (you only get access through a certain workflow which I bypassed by directly editing the role field through npx prisma studio), so when adding a new senior there was an error that came up. However, the database reflects the new senior correctly.

Additional Documentations

Copy link

vercel bot commented Apr 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
the-legacy-project2 ✅ Ready (Inspect) Visit Preview Apr 10, 2024 5:29pm

@cledi01 cledi01 changed the base branch from main to nickbar01234/deprecate-uid April 10, 2024 17:30
Base automatically changed from nickbar01234/deprecate-uid to main April 11, 2024 04:38
@nickbar01234 nickbar01234 force-pushed the senior-form-feedback branch from 44a2914 to 54e012d Compare April 11, 2024 04:41
px-4 py-2 text-[18px] font-normal text-dark-teal drop-shadow-md duration-300 hover:bg-off-white"
type="submit"
style={
edited
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of making it invisible, we should have it dimmed and unclickable.

resolver: zodResolver(seniorFormSchema),
});

const onSubmit: SubmitHandler<SeniorData> = async (data, event) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the data needs to validated before making the API request.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Data is automatically validated because we pass in the zodResolver function to the useForm hook.

firstname: e.target.value,
})
}
{...register("firstname")}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we display error message here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed

const [selectedStudents, setSelectedStudents] = useState<User[]>([]);
const [currentImage, setCurrentImage] = useState<string | StaticImageData>(
ImageIcon
);
const [confirm, setConfirm] = useState<boolean>(false);
const [error, setError] = useState<boolean>(false);
const [edited, setEdited] = useState<boolean>(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this may not be necessary if we still display the save button.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Used to dim the save button now

Copy link
Collaborator

@nickbar01234 nickbar01234 left a comment

Choose a reason for hiding this comment

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

Changes are moving in the right direction! I did a rebase and remove unused variables from AddSenior. A couple details to consider:

  1. I think we should still display the save button as it looks weird without.
  2. Maybe we should considering displaying error messages.
  3. We should update seniorSchema to have a min-required length for firstname and lastname. What do you think would be a good constraint @cledi01?

@cledi01 cledi01 force-pushed the senior-form-feedback branch from 827a39e to 0c6481e Compare April 15, 2024 16:56
@cledi01
Copy link
Collaborator Author

cledi01 commented Apr 15, 2024

Added a constraint for the firstname to have a minimum of 2 characters, I wasn't sure if we needed a last name constraint so I held off on that. Error message also pops up if firstname is less than 2 chars. Submit button is now dimmed and disabled until user makes an edit in the form.

@nickbar01234 nickbar01234 force-pushed the senior-form-feedback branch from 0c6481e to 0a300eb Compare April 16, 2024 00:37
@nickbar01234 nickbar01234 self-requested a review April 16, 2024 00:37
Copy link
Collaborator

@nickbar01234 nickbar01234 left a comment

Choose a reason for hiding this comment

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

Changes are fantastic, thank you! I rebased the branch and updated the logic when we allow users' to submit per discussion.

@nickbar01234 nickbar01234 merged commit 3a65d8f into main Apr 16, 2024
4 checks passed
@nickbar01234 nickbar01234 deleted the senior-form-feedback branch April 16, 2024 00:39
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