Edit basic info modal/kirustar14/personal info modal #43

kirustar14 · 2025-01-22T20:49:48Z

Tracking Info

Resolves #19

Changes

Fixed the edit basic info modal following Figma style.

Testing

Opened the modal in page.tsx and removed it after.

Confirmation of Change

r800360

I went ahead and moved the code from frontend/src/app/Components to frontend/src/components as discussed in Issue #19 Edit Basic Info Modal

To manually test this code, I used the attached frontend/src/app/page.tsx:
"use client";

import Image from "next/image";
import { useState } from "react";
import EditBasicInfoModal from "@/components/EditBasicInfoModal/EditBasicInfoModal";

export default function Home() {
const [isOpen, setIsOpen] = useState(false);

const handleOnSubmit = (data: {
firstName: string;
lastName: string;
email: string;
phone: string;
}) => {
console.log("Submitted Data:", data); // Log to verify form submission
};
return (

<button onClick={() => setIsOpen(true)}>Open Modal
<EditBasicInfoModal
isOpen={isOpen}
onClose={() => setIsOpen(false)}
onSubmit={handleOnSubmit}
/>

);
}

This code generally seems to meet most requirements for Issue #19 on Edit Basic Info Modal except that the component is not sufficiently vertically responsive as demonstrated by the screenshot below.

A fix would be to try to scale the component vertical window size as per the browser vertical window size. This concludes my PR review; I am not requesting changes as this might not be an essential fix.

Miyuki-L

SINCE ROHAN MADE A COMMIT MAKE SURE TO PULL BEFORE PUSHING, IDEALLY PULL BEFORE MAKING PR REVIEW EDITS

Generally a good First Tasks.

CSS needs clean up and simplification and removal of repetition (Let me know if you need help)

Forgot to add in the task before hand (my bad) can you use the following resources below to handle the form input validation (also error messages) and react hook form optimizes things in terms of renderings

Tutorials/Resources
(Handles form field updates) https://react-hook-form.com/
(Input Validation) https://zod.dev/
Tutorial (on reacthook and zod working together)- https://www.freecodecamp.org/news/react-form-validation-zod-react-hook-form/

Also add in an index.ts file un the components folder to export the component (this will clean up the imports in the future)
See Onboarding example: https://github.com/TritonSE/onboarding/blob/main/frontend/src/components/index.ts

Miyuki-L · 2025-01-25T06:05:15Z

frontend/src/components/EditBasicInfoModal/EditBasicInfoModal.css

+  background: white;
+  padding: 40px;
+  border-radius: 15px;
+  width: 657px;


This is a big size to hard code in px. See if you can use rem, em, vw, vh relative units. This will also make the pop up scale to screen size more easily likely addressing the problem Rohan brought up in his review

Miyuki-L · 2025-01-25T06:06:10Z

frontend/src/components/EditBasicInfoModal/EditBasicInfoModal.css

+.close-button {
+  position: absolute;
+  top: 31px;
+  left: 595px;


Same thing try relative units

Miyuki-L · 2025-01-25T06:08:14Z

frontend/src/components/EditBasicInfoModal/EditBasicInfoModal.css

+
+/* Save Button Container */
+.modal-footer button {
+  width: 520.76px;


try relative units

If you are pulling the units from figma 520 is fin don't need to go until 520.76

Miyuki-L · 2025-01-25T06:09:31Z

frontend/src/components/EditBasicInfoModal/EditBasicInfoModal.css

+  font-weight: 400;
+  font-size: 20px;
+  line-height: 26.04px;
+  color: #000000;


can we not just use black?

Miyuki-L · 2025-01-25T06:12:05Z

frontend/src/components/EditBasicInfoModal/EditBasicInfoModal.css

+.phone-group input::placeholder {
+  font-family: "DM Sans", sans-serif;
+  font-weight: 400;
+  font-size: 17px;
+  line-height: 22.13px;
+  color: #b4b4b4;
+}


A lot of repetition in code

Just give the elements with the same stylines the same class name

If need to be slightly different then give it an id attribute (which is unique) to target that specific element

Miyuki-L · 2025-01-25T06:20:58Z

frontend/src/components/EditBasicInfoModal/EditBasicInfoModal.tsx

+            <svg width="14" height="14" viewBox="0 0 14 14" xmlns="http://www.w3.org/2000/svg">
+              <line x1="0" y1="0" x2="14" y2="14" stroke="#000000" strokeWidth="1.5" />
+              <line x1="14" y1="0" x2="0" y2="14" stroke="#000000" strokeWidth="1.5" />
+            </svg>


Create a separate file for this svg under Public/icons

Then import it and use it -- > Max code more readable

Not sure where you code the code from but you can get it straight from the designers figma

<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none"> <path d="M5 19L12 12M12 12L19 5M12 12L5 5M12 12L19 19" stroke="black" stroke-width="1.5" stroke-linecap="round" stroke-linejoin="round"/> </svg> See How they import and use the icon https://github.com/TritonSE/PAP-Inventory-Processing/blob/ea2e7275d40f10fca354eb749f253a52cc75ef60/frontend/src/components/VSRIndividual/VSRIndividualPage/index.tsx#L561-L566

Miyuki-L · 2025-01-25T06:22:17Z

frontend/src/components/EditBasicInfoModal/EditBasicInfoModal.tsx

+const EditBasicInfoModal = ({
+  isOpen,
+  onClose,
+  onSubmit,


onSubmit doesn't need to be passed it

Since what will happen when submitting is sending the data collected in the form over to the backend

nothing exterior is necessary

Miyuki-L · 2025-01-25T06:23:51Z

frontend/src/components/EditBasicInfoModal/EditBasicInfoModal.css

Overall The CSS for the model seems

Top and Left, Height, use for absolute shouldn't be needed so frequently

Usually adding padding and margin to the element and setting their gaps should be sufficient

Miyuki-L · 2025-01-25T06:36:10Z

frontend/src/components/EditBasicInfoModal/EditBasicInfoModal.tsx

+  const [phone, setPhone] = useState("");
+
+  // Modify handleSave to use onSubmit from parent
+  const handleSave = () => {


Put this in a useCallback

useCallback is for optimizing rendering since unless its dependencies change when screen is rerendered it will not recreate the function
Docs: https://react.dev/reference/react/useCallback

Miyuki-L

Looks Great! Just a few more css changes and it should be good.

Miyuki-L · 2025-01-29T20:01:06Z

frontend/src/components/EditBasicInfoModal.tsx

+  phone: z
+    .string()
+    .min(1, "Phone Number is required")
+    .refine((val) => val.length >= 10, {


It seems phonenumbers in the world can be from 5 to 15. We need to take into account phone numbers from different contries.

Miyuki-L · 2025-01-29T20:02:54Z

package-lock.json

There should be no package-lock.json at the root directory. Delete this file

Miyuki-L · 2025-01-29T20:20:31Z

frontend/src/components/EditBasicInfoModal.tsx

+      const response = await fetch("/your-api-endpoint", {
+        method: "POST",
+        headers: {
+          "Content-Type": "application/json",
+        },
+        body: JSON.stringify(data),
+      });


Create this as a function in src/api folder see the onboarding repo for reference. Let's keep all functions that call the backend defiend overthere. and then import and call it here

frontend/src/components/EditBasicInfoModal.tsx

Miyuki-L · 2025-01-29T20:29:27Z

frontend/src/components/EditBasicInfoModal.css

CSS looks a lot cleaner!.

the modal still seems to big interms of the general size of an element.

For the determining relative sizes to use you can write the css start up the frontend inspect that element to see what the resulting size interms of pixels is and compare it to the figma and adjust.

I think the modal should be almost done but we just need to size down everything in the modal

Miyuki-L

Almost there just a few things about the fetch request and resolve the merge conflicts

Miyuki-L · 2025-02-18T07:12:12Z

frontend/src/app/page.tsx

let's keep this page.tsx as is

Miyuki-L · 2025-02-18T07:13:17Z

frontend/src/components/EditBasicInfoModal.tsx

+  //not sure how to send data to backend
+  const onSubmit: SubmitHandler<FormData> = async (data) => {
+    try {
+      const response = await fetchRequest("POST", "/your-api-endpoint", data, {});


set the url endpoint to pull from env file.
https://github.com/TritonSE/PAP-Inventory-Processing/blob/ea2e7275d40f10fca354eb749f253a52cc75ef60/frontend/src/util/validateEnv.ts

Miyuki-L · 2025-02-18T07:17:01Z

frontend/src/app/page.tsx

let's keep this page as is

Miyuki-L · 2025-02-18T07:18:59Z

frontend/src/api/requests.ts

@@ -0,0 +1,23 @@
+type Method = "GET" | "POST" | "PUT";
+
+export async function fetchRequest(


rename this function to be specific to this modal and with more information
i.e. editBasicInfoRequest

The backend route for this functionality should be in. (just a dummy one right now but you should be able to call it

Miyuki-L

1-2 little notes and adjust the spacing for the save button as discussed in the meeting on wednessday.

It so close but you are almost there

Miyuki-L · 2025-02-23T06:25:56Z

frontend/src/components/EditBasicInfoModal.tsx

+    resolver: zodResolver(schema),
+  });
+
+  const onSubmit: SubmitHandler<FormData> = async (data) => {


put this in a usecallback

Miyuki-L · 2025-02-23T06:26:34Z

frontend/package-lock.json

package--lock looks like it took out some dependencies which should still be there. Just npm install again on the frontend folder

Miyuki-L

Looks Good Great!

r800360

I have reviewed this code and tested the Modal and it looked mostly good to me. Nice work on this, Kiruthika!

I realized the phone number validation was particularly weak, so I used a really standard regular expression for this purpose in conjunction with the zod validation in frontend/src/components/EditBasicInfoModal.tsx. I also added the normalization of phone numbers so that the formatting is standard when we store it (removing dots, dashes, spaces, parentheses). I think the minimum phone number length is 7 rather than 5 (ITU-T E.164) so I updated that.

It looks good to me, so I will pass it back to Helen to review and merge.

…nents as discussed in Issue #19 Edit Basic Info Modal

…alize phone number sent to backend

kirustar14 requested a review from Miyuki-L as a code owner January 22, 2025 20:49

Miyuki-L requested review from azpi-arte and r800360 and removed request for azpi-arte January 23, 2025 00:06

kirustar14 linked an issue Jan 23, 2025 that may be closed by this pull request

Edit Basic Info Modal #19

Closed

r800360 reviewed Jan 24, 2025

View reviewed changes

Miyuki-L requested changes Jan 25, 2025

View reviewed changes

Miyuki-L reviewed Jan 29, 2025

View reviewed changes

Miyuki-L requested a review from r800360 February 10, 2025 01:09

Miyuki-L force-pushed the EditBasicInfoModal/kirustar14/PersonalInfoModal branch from ecc859f to daaf5d9 Compare February 18, 2025 07:16

Miyuki-L requested changes Feb 18, 2025

View reviewed changes

Miyuki-L reviewed Feb 23, 2025

View reviewed changes

Miyuki-L approved these changes Feb 24, 2025

View reviewed changes

r800360 approved these changes Feb 27, 2025

View reviewed changes

r800360 requested a review from Miyuki-L February 27, 2025 06:20

kirustar14 and others added 15 commits February 28, 2025 00:38

Edit Basic Info Modal created

836f123

fixed the styling for edit info modal and fixed linting issues

a28c680

Updated EditBasicInfoModal with onSubmit handler

8a22ac8

moved the code from frontend/src/app/Components to frontend/src/compo…

6c3da2d

…nents as discussed in Issue #19 Edit Basic Info Modal

Fixed form validation/css

4de7a32

Remove extraneous package-lock.json

3abf973

fixed modal updates and sized it down

9c53080

fixed modal updated

dab55c5

Fixed up modal Stylings

600c126

fixed fetch request for edit basic info modal

8e8c013

fixed modal

7b6836b

changed red color for error message

eddbc92

frontend(components): adjusted form error message stylings

1204838

frontend(component): update nav bar and have navbar show through modal

991b024

Try to update to fix the linter issue

064d869

r800360 and others added 7 commits February 28, 2025 00:42

Revert parserOptions to fix the linter issue

2d1788a

Revert tsconfig

bd7c886

Remove index.ts and just have index.tsx to fix linter

ca3abdf

Better phone number validation accounting international numbers, norm…

bf47dca

…alize phone number sent to backend

Add eslint disable on phoneRegex

a9ddb9f

Run prettier

d13a555

frontend(root): fixed syntax errors from rebase

273388a

Miyuki-L force-pushed the EditBasicInfoModal/kirustar14/PersonalInfoModal branch from 7aaca08 to 273388a Compare February 28, 2025 08:51

Miyuki-L merged commit c5ef0cf into main Feb 28, 2025
1 of 2 checks passed

Miyuki-L deleted the EditBasicInfoModal/kirustar14/PersonalInfoModal branch February 28, 2025 08:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Edit basic info modal/kirustar14/personal info modal #43

Edit basic info modal/kirustar14/personal info modal #43

kirustar14 commented Jan 22, 2025 •

edited by Miyuki-L

Loading

r800360 left a comment

Miyuki-L left a comment

Miyuki-L Jan 25, 2025

Miyuki-L Jan 25, 2025

Miyuki-L Jan 25, 2025

Miyuki-L Jan 25, 2025

Miyuki-L Jan 25, 2025

Miyuki-L Jan 25, 2025

Miyuki-L Jan 25, 2025

Miyuki-L Jan 25, 2025

Miyuki-L Jan 25, 2025

Miyuki-L left a comment

Miyuki-L Jan 29, 2025

Miyuki-L Jan 29, 2025

Miyuki-L Jan 29, 2025

Miyuki-L Jan 29, 2025

Miyuki-L left a comment •

edited

Loading

Miyuki-L Feb 18, 2025

Miyuki-L Feb 18, 2025

Miyuki-L Feb 18, 2025

Miyuki-L Feb 18, 2025

Miyuki-L Feb 18, 2025

Miyuki-L left a comment

Miyuki-L Feb 23, 2025

Miyuki-L Feb 23, 2025

Miyuki-L left a comment

r800360 left a comment

		@@ -0,0 +1,23 @@
		type Method = "GET" \| "POST" \| "PUT";

		export async function fetchRequest(

Edit basic info modal/kirustar14/personal info modal #43

Edit basic info modal/kirustar14/personal info modal #43

Conversation

kirustar14 commented Jan 22, 2025 • edited by Miyuki-L Loading

Tracking Info

Changes

Testing

Confirmation of Change

r800360 left a comment

Choose a reason for hiding this comment

Miyuki-L left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miyuki-L left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miyuki-L left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miyuki-L left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miyuki-L left a comment

Choose a reason for hiding this comment

r800360 left a comment

Choose a reason for hiding this comment

kirustar14 commented Jan 22, 2025 •

edited by Miyuki-L

Loading

Miyuki-L left a comment •

edited

Loading