-
Notifications
You must be signed in to change notification settings - Fork 40
Kate Marantidi, C23, Bumblebees #33
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
2b866db
9b6b9c7
4f7a8bd
289a2b3
73ba72c
0752ba4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,7 @@ | |
| "test": "vitest --run" | ||
| }, | ||
| "dependencies": { | ||
| "date-fns": "^4.1.0", | ||
|
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. No need to bring in another dependency. |
||
| "luxon": "^2.5.2", | ||
| "react": "^18.3.1", | ||
| "react-dom": "^18.3.1" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,33 @@ | ||
| import './App.css'; | ||
| import entriesData from './data/messages.json'; | ||
| import ChatLog from './components/ChatLog'; | ||
| import { useState } from 'react'; | ||
|
|
||
| const App = () => { | ||
| return ( | ||
| <div id="App"> | ||
| <header> | ||
| <h1>Application title</h1> | ||
| </header> | ||
| <main> | ||
| {/* Wave 01: Render one ChatEntry component | ||
| Wave 02: Render ChatLog component */} | ||
| </main> | ||
| </div> | ||
| ); | ||
| const [entries, setEntries] = useState(entriesData); | ||
| const toggleLike = (id) => { | ||
| const updatedEntries = entries.map(entry => { | ||
| if (entry.id === id) { | ||
| return { ...entry, liked: !entry.liked } ; | ||
| } else{ | ||
| return entry; | ||
| } | ||
| }); | ||
| setEntries(updatedEntries); | ||
|
Comment on lines
+9
to
+16
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. In this case, calculating the next version of the message data using the current state variable and passing that updated version to the state setter shouldn't cause any problems, but we still generally prefer using the callback style of setters. Using that approach, we pass a function to the setter whose parameter will receive the latest state value, and which returns the new value to use for the state. setEntries(entries => entries.map(entry => {
if (entry.id === id) {
return {...entry, liked: !entry.liked};
} else {
return entry;
};
}));We showed this approach 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 totalLikes = entries.filter((entry) => entry.liked).length; | ||
|
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 |
||
|
|
||
| return ( | ||
| <div id="App"> | ||
| <header> | ||
| <h1>Chat between {entries[0].sender} and {entries[1].sender}</h1> | ||
|
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 way to read the participant names from the messages. Notice that this assumes there are only two participants in this conversation, and that they are found in the first two messages. What would happen for other conversation situations?
Some of these cases might not really be workable given the limited data we're working with, but it's worth thinking about what this could look like for a more complete application. |
||
| </header> | ||
| <section id="heartWidget"className="widget">{totalLikes} ❤️{totalLikes !== 1 ? 's' : ''}</section> | ||
|
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.
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 logic to handle pluralization. |
||
| <main> | ||
| <ChatLog entries={entries} onToggleLike={toggleLike} /> | ||
| </main> | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| export default App; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,39 @@ | ||
| import './ChatEntry.css'; | ||
| import PropTypes from 'prop-types'; | ||
| import TimeStamp from './TimeStamp'; | ||
|
|
||
| const ChatEntry = () => { | ||
| return ( | ||
| <div className="chat-entry local"> | ||
| <h2 className="entry-name">Replace with name of 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> | ||
| </section> | ||
| </div> | ||
| ); | ||
|
|
||
| const ChatEntry = (props) => { | ||
| const localSender = props.sender === 'Vladimir'; | ||
| const entryClass = `chat-entry ${localSender ? 'local' : 'remote'}`; | ||
|
Comment on lines
+7
to
+8
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 logic to decide whether to treat a message as local or remote. How could we generalize this so that it didn't need to look explicitly for Vladimir? This project only passes in a single conversation, but ideally, our components should work in other situations. In the general case, does the |
||
| // const likedStatus = () => { | ||
| // props.liked ? 'liked' : 'not-liked'; | ||
|
|
||
| return ( | ||
| <div className={entryClass} key={props.id}> | ||
| <h2 className="entry-name">{props.sender}</h2> | ||
| <section className="entry-bubble"> | ||
| <p>{props.body}</p> | ||
| <p className="entry-time"><TimeStamp time={props.timeStamp} /></p> | ||
|
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 |
||
| <button className="like" onClick={() => props.onToggleLike(props.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. 👍 Passing the |
||
| {props.liked ? '❤️' : '🤍'} | ||
|
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 a ternary to pick the emoji to show for the liked status. Consider moving even something small like this to a helper function external to the component, so that the behavior of the emoji-picking logic can be tested easily outside of React. |
||
| </button> | ||
| </section> | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| 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, | ||
| onToggleLike: PropTypes.func, //had to remove isRequired to pass the test from Waves 1 and 2 | ||
|
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 If we do leave props marked not |
||
| }; | ||
|
|
||
| ChatEntry.defaultProps = { // added dafault props so it stays always defined | ||
| onToggleLike: () => {}, | ||
| }; | ||
|
Comment on lines
+35
to
37
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. React reports a warning about |
||
|
|
||
| export default ChatEntry; | ||
| export default ChatEntry; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| // import entries from './data/messages.json'; | ||
| import PropTypes from 'prop-types'; | ||
| import ChatEntry from './ChatEntry'; | ||
|
|
||
| const ChatLog = ({ entries, onToggleLike }) => { | ||
|
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.
|
||
| return ( | ||
| <div id="ChatLog"> | ||
| {entries.map((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. Nice use of |
||
| <ChatEntry key={entry.id} {...entry} onToggleLike={onToggleLike} /> | ||
|
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. 🔎 While spreading to pass props can reduce perceived redundancy, React style practices generally recommend passing the props explicitly. Spreading can lead to unintended data being passed, and obscures what values are being provided to the component. It's not a problem for a small example like this project, but it can make it harder to keep track of data in larger applications. PropTypes (or TS types going forward) can help us catch cases where a breaking change might occur, but unless working on a team with a style guide that favors using spreading, prefer to explicitly break out the props. |
||
| ))} | ||
| </div> | ||
| ); | ||
| }; | ||
| 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, | ||
| onToggleLike: PropTypes.func.isRequired, | ||
|
Comment on lines
+15
to
+24
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. Similar to the props for 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. |
||
| }; | ||
|
|
||
| export default ChatLog; | ||
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.
What led you to change this? There are definitely arguments for using tabs rather than spaces, but in that case, what tab width are you using locally? GitHub assumes 8 spaces (!) for tabs, which would be deep even for Python, but is especially cumbersome in JS, where there is a lot of indentation due to nested anonymous functions.