Skip to content

React Eval#19

Open
acl13 wants to merge 33 commits into
projectshft:mainfrom
acl13:main
Open

React Eval#19
acl13 wants to merge 33 commits into
projectshft:mainfrom
acl13:main

Conversation

@acl13
Copy link
Copy Markdown

@acl13 acl13 commented Sep 17, 2024

No description provided.

acl13 added 30 commits September 9, 2024 13:40
Comment thread app/data/contactAPI.js
@@ -0,0 +1,38 @@
/**
* The ContactAPI is a JavaScript object that contains an array to hold the contact objects,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

great comment

Comment thread app/data/contactAPI.js

export const ContactAPI = {
contacts: [],
all: function () {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

don't be afraid of being even more clear

Suggested change
all: function () {
allContacts: function () {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Why not es6?
use arrow functions!

Comment thread app/data/contactAPI.js
this.contacts.push({ id, name, email, phone_number, image_url });
},
remove: function (contact) {
const index = this.contacts.findIndex((c) => c.id === contact.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.

be more clear, this not just the index its:

Suggested change
const index = this.contacts.findIndex((c) => c.id === contact.id);
const contactIdIndex = this.contacts.findIndex((c) => c.id === contact.id);

Comment thread app/data/contactAPI.js
];
// removes contact from array
this.contacts.splice(index, 1);
return trimmedContacts;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

not sure why you need to return here trimmed contacts vs other functions you return this.contacts.

Comment thread app/data/contactAPI.js
Comment on lines +29 to +32
this.contacts[index].name = name;
this.contacts[index].email = email;
this.contacts[index].phone_number = phone_number;
this.contacts[index].image_url = image_url;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

not performant, you can optimize this with object deconstruction. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment

* The edit page will display a ContactForm component that is already populated with the information of contact that matches the id of the route
**/

export default function EditContact() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

dont name the files page.js, use the proper name and no need to nest under an id folder.
editContact.js

Comment thread app/components/Contact.js
height={50}
unoptimized
onError={(e) =>
(e.target.src =
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

love the default

const [email, setEmail] = useState(initialEmail);
const [phone_number, setPhone_number] = useState(initialPhone);
const [image_url, setImage_url] = useState(initialImgUrl);
const generateId = () => Math.round(Math.random() * 100000000);
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 should be in a utils separate file and not here because will recreate every time you access the page


const errors = {};

if (name === null || !name.trim()) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

big yes to .trim()

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