Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a React-based chat application UI featuring a conversation between Vladimir and Estragon. The implementation includes creating the ChatLog component to render multiple chat entries, updating the ChatEntry component to display individual messages with like functionality, and adding state management in App.jsx to handle the like feature and count total likes.
- Creates the ChatLog component to map over and render multiple ChatEntry components
- Updates ChatEntry to accept and display props (sender, body, timestamp) with a toggleable like button
- Implements state management and like counting logic in App.jsx
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/components/ChatLog.jsx | New component that maps chat entries array to ChatEntry components with PropTypes validation |
| src/components/ChatEntry.jsx | Updated to accept props, render dynamic content, integrate TimeStamp component, and implement like button functionality |
| src/App.jsx | Added state management for chat data, like toggle handler, total likes counter, and ChatLog component integration |
| src/components/ChatEntry.css | Minor formatting fix (added newline at end of file) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| // <ChatEntry sender = {message1.sender} body = {message1.body} timeStamp = '2018-05-29T22:49:06+00:00' > | ||
| // </ChatEntry> | ||
| /* Wave 01: Render one ChatEntry component | ||
| messages*/ | ||
| } |
There was a problem hiding this comment.
Commented out code should be removed before merging. These leftover comments from Wave 01 are no longer needed since the implementation is complete with the ChatLog component.
| { | |
| // <ChatEntry sender = {message1.sender} body = {message1.body} timeStamp = '2018-05-29T22:49:06+00:00' > | |
| // </ChatEntry> | |
| /* Wave 01: Render one ChatEntry component | |
| messages*/ | |
| } | |
There was a problem hiding this comment.
Commented out code is confusing to other devs who might not understand why the code has been left in if it's no longer needed. It's also prone to introducing bugs because the code could get uncommented and the code would unintentionally execute.
It also compromises your codebase's readability by adding clutter and making it less concise.
| messages*/ | ||
| } | ||
| <ChatLog | ||
| entries = {chatData} |
There was a problem hiding this comment.
Inconsistent spacing around equals signs in JSX props. The code uses entries = {chatData} with spaces, but standard JSX formatting omits spaces (e.g., entries={chatData}). This is inconsistent with the prop assignments in ChatLog.jsx (lines 8-14).
| entries = {chatData} | |
| entries={chatData} |
| import ChatLog from './components/ChatLog'; | ||
| import { useState } from 'react'; | ||
|
|
||
| const countTotalLikes = (chatData) =>{ |
There was a problem hiding this comment.
Arrow function formatting issue: missing spaces around the arrow operator. Should be (chatData) => { instead of (chatData) =>{ to follow JavaScript formatting conventions and maintain consistency with the rest of the codebase.
| const countTotalLikes = (chatData) =>{ | |
| const countTotalLikes = (chatData) => { |
| const[chatData, setChatData] = useState(messages); | ||
| const totalLikes = countTotalLikes(chatData); | ||
|
|
||
| const handleToggleLike = id =>{ |
There was a problem hiding this comment.
Arrow function formatting issue: missing space after id and before the arrow operator. Should be id => { instead of id =>{ to follow JavaScript formatting conventions and maintain consistency.
| const handleToggleLike = id =>{ | |
| const handleToggleLike = id => { |
There was a problem hiding this comment.
Take care to pay attention to your code style while you're writing code or take some time to look over your work before opening a pull request for review.
In industry, it would be frowned upon to request a review for code with style issues. these issues should be cleaned up beforehand.
| const handleToggleLike = id =>{ | ||
| setChatData(chatData => { | ||
| return chatData.map(chat => { | ||
| if(chat.id === id) { |
There was a problem hiding this comment.
Missing space in conditional statement. Should be if (chat.id === id) instead of if(chat.id === id) to follow JavaScript formatting conventions and maintain readability.
| if(chat.id === id) { | |
| if (chat.id === id) { |
| <button className="like">🤍</button> | ||
| <p>{body}</p> | ||
| <p className="entry-time"><TimeStamp time = {timeStamp}></TimeStamp></p> | ||
| <button className="like" onClick={()=>toggleLike(id)}>{heartColor}</button> |
There was a problem hiding this comment.
Arrow function formatting issue: missing space before the arrow operator. The function should be written as () => toggleLike(id) rather than ()=>toggleLike(id) to follow standard JavaScript formatting conventions and maintain consistency with the codebase (see App.jsx lines 6, 16, and throughout the file).
| <button className="like" onClick={()=>toggleLike(id)}>{heartColor}</button> | |
| <button className="like" onClick={() => toggleLike(id)}>{heartColor}</button> |
| timeStamp: PropTypes.string.isRequired, | ||
| liked: PropTypes.bool.isRequired, | ||
| })).isRequired, | ||
| toggleLike: PropTypes.func, |
There was a problem hiding this comment.
The toggleLike prop should be marked as isRequired to match the usage in ChatEntry component where it's called unconditionally. Without this prop, the ChatEntry component will throw a runtime error when the like button is clicked.
| toggleLike: PropTypes.func, | |
| toggleLike: PropTypes.func.isRequired, |
| <p className="entry-time">Replace with TimeStamp component</p> | ||
| <button className="like">🤍</button> | ||
| <p>{body}</p> | ||
| <p className="entry-time"><TimeStamp time = {timeStamp}></TimeStamp></p> |
There was a problem hiding this comment.
Inconsistent spacing around the equals sign. The code uses time = {timeStamp} with spaces, but the convention in JSX is typically to omit spaces around equals signs in props (like time={timeStamp}). This is inconsistent with the prop assignments in ChatLog.jsx (lines 8-14) and elsewhere in this file.
| <p className="entry-time"><TimeStamp time = {timeStamp}></TimeStamp></p> | |
| <p className="entry-time"><TimeStamp time={timeStamp}></TimeStamp></p> |
| body: PropTypes.string.isRequired, | ||
| timeStamp: PropTypes.string.isRequired, | ||
| liked: PropTypes.bool.isRequired, | ||
| toggleLike: PropTypes.func, |
There was a problem hiding this comment.
The toggleLike prop should be marked as isRequired since the component uses it without any null/undefined check in the onClick handler (line 13). If toggleLike is undefined, clicking the like button will cause a runtime error. Mark it as .isRequired or add a conditional check before usage.
| toggleLike: PropTypes.func, | |
| toggleLike: PropTypes.func.isRequired, |
| import PropTypes from 'prop-types'; | ||
|
|
||
| const ChatEntry = () => { | ||
| const ChatEntry = ({ id, sender, body, timeStamp, liked, toggleLike }) => { |
There was a problem hiding this comment.
Nice job destructuring your different props. This is beneficial for a few reasons:
- you know every possible prop right away
- you know when a prop is being unused, in order to remove it
- knowing which props are being used will also remind you what you need to validate
| <p className="entry-time">Replace with TimeStamp component</p> | ||
| <button className="like">🤍</button> | ||
| <p>{body}</p> | ||
| <p className="entry-time"><TimeStamp time = {timeStamp}></TimeStamp></p> |
There was a problem hiding this comment.
Nice job using the provided TimeStamp component 👍
| const ChatLog = ({ entries, toggleLike }) => { | ||
| const chatComponents = entries.map(entry => { | ||
| return (<ChatEntry | ||
| key={entry.id} |
There was a problem hiding this comment.
👍 The key attribute is important for React to be able to detect certain kinds of data changes in an efficient manner. We're also using the id for our own id prop, so it might feel redundant to pass both, but one is for our logic and one is for React internals.
| import ChatLog from './components/ChatLog'; | ||
| import { useState } from 'react'; | ||
|
|
||
| const countTotalLikes = (chatData) =>{ |
There was a problem hiding this comment.
❗️👀 Functions that are only used within a specific component, like countTotalLikes is only used inside the App component on line 14, should be defined inside the component to make the code more readable because it keeps related logic together.
Therefore, this function should be defined in the App component (like you do with handleToggleLike on line 16).
| const[chatData, setChatData] = useState(messages); | ||
| const totalLikes = countTotalLikes(chatData); | ||
|
|
||
| const handleToggleLike = id =>{ |
There was a problem hiding this comment.
Take care to pay attention to your code style while you're writing code or take some time to look over your work before opening a pull request for review.
In industry, it would be frowned upon to request a review for code with style issues. these issues should be cleaned up beforehand.
| setChatData(chatData => { | ||
| return chatData.map(chat => { |
There was a problem hiding this comment.
Nice job using the callback-style for updating the state.
| <div id="App"> | ||
| <header> | ||
| <h1>Application title</h1> | ||
| <h1>Chat between Vladimir and Estragon</h1> |
There was a problem hiding this comment.
Can you think of a way to dynamically get the senders' names without hardcoding "Vladimir" and "Estragon" here?
| { | ||
| // <ChatEntry sender = {message1.sender} body = {message1.body} timeStamp = '2018-05-29T22:49:06+00:00' > | ||
| // </ChatEntry> | ||
| /* Wave 01: Render one ChatEntry component | ||
| messages*/ | ||
| } |
There was a problem hiding this comment.
Commented out code is confusing to other devs who might not understand why the code has been left in if it's no longer needed. It's also prone to introducing bugs because the code could get uncommented and the code would unintentionally execute.
It also compromises your codebase's readability by adding clutter and making it less concise.
| // Replace the outer tag name with a semantic element that fits our use case | ||
| <replace-with-relevant-semantic-element className="chat-entry local"> | ||
| <h2 className="entry-name">Replace with name of sender</h2> | ||
| <article className="chat-entry local"> |
There was a problem hiding this comment.
It was an optional enhancement, but how could you determine if a chat was from a 'local' sender vs a 'remote' sender so that the chat bubbles would have different styles applied to it?
If you have time to re-visit this project for React.js review, it would be good practice to try applying local vs. remote classes to the components so they render differently depending on who sent the message.
| } | ||
| <ChatLog | ||
| entries = {chatData} | ||
| toggleLike={handleToggleLike}> </ChatLog> |
There was a problem hiding this comment.
Prefer to have the closing tag on its own line (like how you put the opening tag on its own line) to help with readability.
No description provided.