I am trying to learn React basics and completed a chat app. My main purpose was to create user-specific text areas and preserve the draft when changing between people. Beforehand I had some trouble with the ContactList and posted it on StackOverflow. Thankfully it got solved and I finished this assignment. I don't know if the code is any good, what can I do to improve it and are there any logical mistakes? I only used React documentation and Stack Overflow to get help. This is my first solo project so I am open to any improvements and suggestions.
//App.jsx
import { useState } from 'react'
import ContactList from './ContactList';
import './App.css'
import Chat from './Chat';
export default function App() {
const [selectedContact, setSelectedContact] = useState(() => contacts[0]);
const [messages, setMessages] = useState({
0:"",
1:"",
2:"",
})
function handleTextChange(e){
setMessages({
...messages,
[selectedContact.id]: e.target.value
})
}
function handleButtonClick(contact , event){
event.preventDefault();
setSelectedContact(contact);
}
return (
<div className='container'>
<ContactList handleButtonClick={handleButtonClick} contacts={contacts} />
<Chat contact={selectedContact} handleTextChange={handleTextChange} messages={messages}/>
</div>
)
}
const contacts = [
{id: 0, name: 'Taylor', email: '[email protected]'},
{id: 1, name: 'Alice', email: '[email protected]'},
{id: 2, name: 'Bob', email: '[email protected]'},
];
// Chat.jsx
export default function Chat({ contact, handleTextChange, messages }) {
return (
<div className="input-area">
<textarea
className="text-area"
placeholder={"Chat to " + contact.name}
value={messages[contact.id]}
onChange={handleTextChange}
></textarea>
<button key={contact.id} className="personName">
Send to {contact.email}
</button>
</div>
);
}
//ContactList.jsx
export default function ContactList({ handleButtonClick, contacts }) {
return (
<div className="contact-buttons">
{contacts.map((contact) => {
return (
<button
key={contact.id}
className="personName"
onClick={(event) => handleButtonClick(contact , event)}
>
{contact.name}
</button>
);
})}
</div>
);
}
1 Answer 1
Not bad for an assignment. Just going top to bottom:
App.tsx
You reference contacts
before defining it. This works in JS because the declaration is hoisted, and the function referencing it doesn't get called until after contacts
is initialized. Though it works, it's bad practice and can complicate debugging. I would move contacts
to above App
.
Not sure why you use an initializer function to initialize the selectedContact
state. You can just initialize it directly like this: useState(contacts[0])
When initializing messages
, you copy-paste the same IDs as in contacts
. You should build it using the actual contacts
instead, maybe something like this:
useState(
contacts
.map(contact => contact.id)
.reduce(
(acc, id) => {
acc[id] = '';
return acc;
},
/* initialValue= */ {}
)
);
But it's bad practice to use numbers as object keys. Almost always an object key will be a string. For messages
, you're better off keeping an array of objects like [{id: 0, message: ''}]
, or even using a Map
.
In handleTextChange
, you set the state using the previous state directly. In more complex applications, this can create race conditions where your state changes can overwrite each other. To avoid this, set this state using an updater function, like this:
setMessages(prevMessages => ({
...prevMessages,
[selectedContact.id]: e.target.value,
}));
Chat.jsx
Your component doesn't need to know about the entire messages
object. It clutters the information present and in more complex applications may cause unnecessary rerenders. Prefer accepting a single message
and calling your component with messages[contact.id]
.
ContactList.jsx
When iterating contacts
, for arrow functions that directly return a value, you can skip the braces and return
. Like this:
{contacts.map((contact) => (
<button ... >
...
</button>
))}
This is the opposite case. When defining onClick
, you expect handleButtonClick
to return nothing. But this kind of usage can be dangerous if it unexpectedly returns a value. You should wrap the call in braces to make this explicit, like this:
onClick={(event) => {
handleButtonClick(contact, event);
}}
Overall
Your code has stylistic inconsistencies and is missing some typical stylistic conventions for JS. For example:
- Trailing commas in some objects, not in others
- Semicolons in some places, not in others
- 2-space indents in some places, 4-space in others
- Whitespace separating some function declarations, not others
- Other inconsistent whitespace elsewhere
A good linter/formatter can fix most of these automatically. If you use VS Code, it should come by default.
-
\$\begingroup\$ Thank you for your time and valuable comments about my project. I will try to avoid these mistakes in my new projects. Thank you for giving me the opportunity to become a better developer. Have a nice day. \$\endgroup\$Crehera– Crehera2024年02月02日 10:05:14 +00:00Commented Feb 2, 2024 at 10:05
-
\$\begingroup\$ @Crehara No problem. I made these same mistakes too when I first started out, as did everyone else. Practice and feedback will make your projects better. \$\endgroup\$user3932000– user39320002024年02月02日 11:50:55 +00:00Commented Feb 2, 2024 at 11:50