-
Notifications
You must be signed in to change notification settings - Fork 35
Possum - Wenxin Li #16
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
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 |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| { | ||
| "singleQuote": true, | ||
| "semi": true | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,31 @@ | ||
| import './App.css'; | ||
| import ChatLog from './components/ChatLog'; | ||
| import messages from './data/messages.json'; | ||
| import { useState } from 'react'; | ||
|
|
||
| const App = () => { | ||
| const [entries, setEntries] = useState(messages); | ||
|
|
||
| const handleOnLike = (id) => { | ||
| setEntries((prevEntries) => | ||
| prevEntries.map((entry) => | ||
| entry.id === id ? { ...entry, liked: !entry.liked } : entry | ||
|
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. We showed toggling the particular value directly in the handler in class, but technically, we're mixing a few responsibilities here. Rather than this function needing to know how to change the liked status itself, we could move this update logic to a helper function. This would better mirror how we eventually update records when there's an API call involved. In this project, our messages are very simple objects, but if we had more involved operations, it could be worthwhile to create an actual class with methods to work with them, or at least have a set of dedicated helper functions to centralize any such mutation logic. |
||
| ) | ||
| ); | ||
| }; | ||
|
|
||
| const handlerCountLikes = () => { | ||
|
||
| return entries.filter((entry) => entry.liked).length; | ||
| }; | ||
|
Comment on lines
+17
to
+19
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 determining the total likes based on the like data of each message. We don't need an additional piece of state to track this, since it can be derived from the existing state we are tracking. Great idea to filter down the data to the liked messages and use the list length as the count. This is a really understandable alternative to the more canonical Consider not using Note that if we passed the messages in as a parameter rather than depending on having them be visible in the enclosing scope, we could actually move this logic entirely out of the React component, making this business logic more testable. |
||
|
|
||
| return ( | ||
| <div id="App"> | ||
| <header> | ||
| <h1>Application title</h1> | ||
| <h1>Chat between Vladimir and Estragon</h1> | ||
| <h2>{handlerCountLikes()} ❤️s</h2> | ||
| </header> | ||
| <main> | ||
| {/* Wave 01: Render one ChatEntry component | ||
| Wave 02: Render ChatLog component */} | ||
| <ChatLog entries={entries} onLike={handleOnLike} /> | ||
| </main> | ||
| </div> | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,21 +1,34 @@ | ||
| import './ChatEntry.css'; | ||
| import PropTypes from 'prop-types'; | ||
| import TimeStamp from './TimeStamp'; | ||
|
|
||
| const ChatEntry = () => { | ||
| const ChatEntry = ({ id, sender, body, timeStamp, onLike, 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"> | ||
| <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} /> | ||
|
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 use of the supplied |
||
| </p> | ||
|
|
||
| <button className="like" onClick={() => onLike(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.
|
||
| {liked ? '❤️' : '🤍'} | ||
| </button> | ||
| </section> | ||
| </replace-with-relevant-semantic-element> | ||
| </article> | ||
| ); | ||
| }; | ||
|
|
||
| ChatEntry.propTypes = { | ||
| // Fill with correct proptypes | ||
| id: PropTypes.number.isRequired, | ||
| sender: PropTypes.string.isRequired, | ||
| body: PropTypes.string.isRequired, | ||
| timeStamp: PropTypes.string.isRequired, | ||
| liked: PropTypes.bool.isRequired, | ||
| onLike: PropTypes.func.isRequired, | ||
|
Comment on lines
+26
to
+31
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 The remaining props were up to you, and the tests don't know about them. As a result, using To properly mark any other props Alternatively, for any props that we leave not required, we should also have logic in our component to not try to use the value if it's undefined. |
||
| }; | ||
|
|
||
| export default ChatEntry; | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,38 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import './ChatLog.css'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import PropTypes from 'prop-types'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import ChatEntry from './ChatEntry'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const ChatLog = ({ entries, onLike }) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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. A fragment Here, |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <ul> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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. We should keep in mind the overall HTML constructed by our components when picking the HTML tags. We have <ul>
<article>...</article>
<article>...</article>
...
</ul>
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| {entries.map((chatEntry) => ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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 use of |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <ChatEntry | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| key={chatEntry.id} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id={chatEntry.id} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+11
to
+12
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 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sender={chatEntry.sender} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| body={chatEntry.body} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| timeStamp={chatEntry.timeStamp} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| liked={chatEntry.liked} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| onLike={onLike} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ))} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </ul> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| </> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+7
to
+21
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| <> | |
| <ul> | |
| {entries.map((chatEntry) => ( | |
| <ChatEntry | |
| key={chatEntry.id} | |
| id={chatEntry.id} | |
| sender={chatEntry.sender} | |
| body={chatEntry.body} | |
| timeStamp={chatEntry.timeStamp} | |
| liked={chatEntry.liked} | |
| onLike={onLike} | |
| /> | |
| ))} | |
| </ul> | |
| </> | |
| <ul> | |
| {entries.map((chatEntry) => ( | |
| <ChatEntry | |
| key={chatEntry.id} | |
| id={chatEntry.id} | |
| sender={chatEntry.sender} | |
| body={chatEntry.body} | |
| timeStamp={chatEntry.timeStamp} | |
| liked={chatEntry.liked} | |
| onLike={onLike} | |
| /> | |
| ))} | |
| </ul> |
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.
This comment from Copilot is pointing out what I mentioned above about not needing the fragment <>.
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.
Similar to the props for ChatEntry, here, the entries prop is included in the tests, but the like toggle is not, resulting in prop warnings (unless we update the tests to reflect our custom props).
Again, if we were to leave this as not required so as to avoid the test warnings, we'd want to be sure that all the script logic in our component worked properly even in the absence of this value.
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 use of the callback setter style. In this application, it doesn't really matter whether we use the callback style or the value style, but it's good practice to get in the habit of using the callback style.