Skip to content

contact-list project submission#17

Open
colormethanh wants to merge 13 commits into
projectshft:mainfrom
colormethanh:main
Open

contact-list project submission#17
colormethanh wants to merge 13 commits into
projectshft:mainfrom
colormethanh:main

Conversation

@colormethanh
Copy link
Copy Markdown

No description provided.


export const useContacts = () => useContext(ContactsContext);

const initialContacts = [{
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ideally this will come from a different file like a utils


const createID = () => Math.round(Math.random() * 100000000);

const addContact = (data) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

data is too generic, use something more descriptive for param name

Suggested change
const addContact = (data) => {
const addContact = (contactData) => {


const deleteContact = (id) => {
const newContacts = [];
contacts.forEach((contact) => contact.id !== parseInt(id) && newContacts.push(contact));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ideally, you will pass id already parsed so its ready to use, when you start using typescript you will understand properly why.

return setContacts(newContacts);
};

const editContact = (formData, id) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the function name does not match what the function is doing, if you have a bug by the setContacts function call, it will be hard to track as it goes undetected here. better to separate responsibilities

return {...formData, id};
};

const getPrevAndNext = (id) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

getprevandnext what?

Suggested change
const getPrevAndNext = (id) => {
const getPrevAndNextIds = (id) => {

label="Phone Number"
name="phone_number"
type="tel"
attributes={{pattern: "[0-9]{3}-[0-9]{3}-[0-9]{4}"}}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this patter is not universal

<th scope="col"> Name </th>
<th scope="col"> Email </th>
<th scope="col"> Phone </th>
<th scope="col"></th>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

here is an example where missing comments help, why do you have this empty column? I have to go to the data to notice its for the delete btn

};

return (
<tr onClick={(e) => (handleClick(e))} style={{"cursor": "pointer"}} id={`tableRow${person.id}`}>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

better to stick to classes instead of style

Comment on lines +22 to +28

<h1 className="text-center"> Create New Contact </h1>
<ContactForm
formData={formData}
className="col-8 col-md-6 mx-auto"
handleInput={handleInput}
handleSubmit={(e) => handleSubmit(e)} />
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

check your code alignment

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