-
Notifications
You must be signed in to change notification settings - Fork 35
Possum - Geethalekshmi #28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
bb2c2c1
89db812
46f1e64
e9cadd2
09a9738
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,17 +1,50 @@ | ||||||||||||||||
| import './App.css'; | ||||||||||||||||
| import messages from './data/messages.json'; | ||||||||||||||||
| import ChatLog from './components/ChatLog'; | ||||||||||||||||
| import { useState } from 'react'; | ||||||||||||||||
|
|
||||||||||||||||
| const countTotalLikes = (chatData) =>{ | ||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❗️👀 Functions that are only used within a specific component, like Therefore, this function should be defined in the |
||||||||||||||||
| return chatData.reduce((total, chat) => { | ||||||||||||||||
| return total + (chat.liked ? 1 : 0); | ||||||||||||||||
| }, 0); | ||||||||||||||||
| }; | ||||||||||||||||
|
|
||||||||||||||||
| const App = () => { | ||||||||||||||||
| const[chatData, setChatData] = useState(messages); | ||||||||||||||||
| const totalLikes = countTotalLikes(chatData); | ||||||||||||||||
|
|
||||||||||||||||
| const handleToggleLike = id =>{ | ||||||||||||||||
|
||||||||||||||||
| const handleToggleLike = id =>{ | |
| const handleToggleLike = id => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job using the callback-style for updating the state.
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you think of a way to dynamically get the senders' names without hardcoding "Vladimir" and "Estragon" here?
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,4 +97,4 @@ button { | |
|
|
||
| .chat-entry.remote .entry-bubble:hover::before { | ||
| background-color: #a9f6f6; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,21 +1,28 @@ | ||||||
| import './ChatEntry.css'; | ||||||
| import TimeStamp from './TimeStamp.jsx'; | ||||||
| import PropTypes from 'prop-types'; | ||||||
|
|
||||||
| const ChatEntry = () => { | ||||||
| const ChatEntry = ({ id, sender, body, timeStamp, liked, toggleLike }) => { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice job destructuring your different props. This is beneficial for a few reasons:
|
||||||
| const heartColor = liked ? '❤️' : '🤍'; | ||||||
| return ( | ||||||
| // 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
| <h2 className="entry-name">{sender}</h2> | ||||||
| <section className="entry-bubble"> | ||||||
| <p>Replace with body of ChatEntry</p> | ||||||
| <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> | ||||||
|
||||||
| <p className="entry-time"><TimeStamp time = {timeStamp}></TimeStamp></p> | |
| <p className="entry-time"><TimeStamp time={timeStamp}></TimeStamp></p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job using the provided TimeStamp component 👍
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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> |
Copilot
AI
Dec 16, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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, |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,38 @@ | ||||||
| import ChatEntry from './ChatEntry.jsx'; | ||||||
| import './ChatLog.css'; | ||||||
| import PropTypes from 'prop-types'; | ||||||
|
|
||||||
| const ChatLog = ({ entries, toggleLike }) => { | ||||||
| const chatComponents = entries.map(entry => { | ||||||
| return (<ChatEntry | ||||||
| key={entry.id} | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 The |
||||||
| id={entry.id} | ||||||
| sender={entry.sender} | ||||||
| body={entry.body} | ||||||
| timeStamp={entry.timeStamp} | ||||||
| liked={entry.liked} | ||||||
| toggleLike={toggleLike}> | ||||||
| </ChatEntry> | ||||||
| ); | ||||||
| }); | ||||||
| return ( | ||||||
| <> | ||||||
| {chatComponents} | ||||||
| </> | ||||||
|
|
||||||
| ); | ||||||
| }; | ||||||
|
|
||||||
| ChatLog.propTypes = { | ||||||
| entries: PropTypes.arrayOf(PropTypes.shape({ | ||||||
| id: PropTypes.number.isRequired, | ||||||
| sender: PropTypes.string.isRequired, | ||||||
| body: PropTypes.string.isRequired, | ||||||
| timeStamp: PropTypes.string.isRequired, | ||||||
| liked: PropTypes.bool.isRequired, | ||||||
| })).isRequired, | ||||||
| toggleLike: PropTypes.func, | ||||||
|
||||||
| toggleLike: PropTypes.func, | |
| toggleLike: PropTypes.func.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.