-
Notifications
You must be signed in to change notification settings - Fork 44
C22 -Phoenix -- Beenish Ali #35
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
3101aa6
f40e007
d3c6871
3a7c37c
fa32bbf
f85d7d6
e7ba0f3
c317629
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,16 +1,48 @@ | ||
| import './App.css'; | ||
| import Data from './data/messages.json'; | ||
| import ChatLog from './components/ChatLog'; | ||
| import { useState } from 'react'; | ||
|
|
||
|
|
||
| function App () { | ||
| const [messageData, setMessageData] = useState(Data); | ||
|
|
||
| const senders = [...new Set(Data.map(message => message.sender))]; | ||
|
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 |
||
|
|
||
| const handleLiked = (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. 👍 Since our state is defined here, we also need to define our mutating function here so that it can "see" the setter function. All we need to receive is the |
||
| setMessageData(messageData => messageData.map(message => { | ||
|
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 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. 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 map here to both handle making a new list so that React sees the message data has changed, and make new data for the clicked message with its like status toggled. |
||
| if (message.id === id) { | ||
| return {...message, liked: !message.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. 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. |
||
| } else { | ||
| return message; | ||
| }; | ||
| })); | ||
| }; | ||
|
|
||
| const calcTotalLiked = (messageData) => { | ||
| let total = 0; | ||
| for (const message of messageData) { | ||
| if (message.liked) { | ||
| total += 1; | ||
| } | ||
| }; | ||
| return total; | ||
| }; | ||
|
|
||
| const totalLiked = calcTotalLiked(messageData); | ||
|
Comment on lines
+22
to
+32
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. Explicitly totalling the count is perfectly fine, but many JS programmers would use const calcTotalLiked = (messageData) => {
return messageData.reduce((acc, message) => {
return message.liked ? acc + 1 : acc;
}, 0);
};The first few times we work with |
||
|
|
||
| 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> | ||
| <> | ||
| <div id="App"> | ||
| <header> | ||
| <h1>Chat between {senders[0]} and {senders[1]}</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. For the conversation in the project, there are exactly two participants and both have sent messages, which is the only reason we can find them. 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. |
||
| <h2>{totalLiked} ❤️s</h2> | ||
| </header> | ||
| <main> | ||
| <ChatLog entries = {messageData} onLiked={handleLiked}/> | ||
| </main> | ||
| </div> | ||
| </> | ||
| ); | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,20 +1,43 @@ | ||
| import './ChatEntry.css'; | ||
| import PropTypes from 'prop-types'; | ||
| import TimeStamp from './TimeStamp'; | ||
|
|
||
| const ChatEntry = ({sender, body, timeStamp, id, liked, onLiked}) => { | ||
|
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. 👍 I like using destructured props to make it more visibly clear in the function definition itself what props we're expecting to receive. We do need to remember that these are all passed in as a single object (the one we usually call |
||
| const heart = 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. 👍 We can figure out which emoji to use for the liked status based on the |
||
|
|
||
| const localOrRemote = sender === 'Vladimir' ? 'chat-entry local' : 'chat-entry remote'; | ||
|
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 onLikeClicked = () => { | ||
| onLiked(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 |
||
| }; | ||
|
|
||
| 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> | ||
| <> | ||
| <div className={localOrRemote}> | ||
| <h2 className="entry-name">{sender}</h2> | ||
| <section className="entry-bubble"> | ||
| <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 Nit: no spaces around attribute |
||
| </p> | ||
| <button | ||
| className="like" | ||
| onClick={onLikeClicked}> | ||
|
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 need a wrapper of some kind rather than calling the received callback through props, since our callback function is expecting a message id as its parameter. If we tried to use it directly as the click event handler, React would end up passing it a clink event, since any function registered as an event handler will always be given the event detail information as its argument. |
||
| {heart} | ||
| </button> | ||
| </section> | ||
| </div> | ||
| </> | ||
| ); | ||
| }; | ||
|
|
||
| ChatEntry.propTypes = { | ||
| // Fill with correct proptypes | ||
| sender: PropTypes.string.isRequired, | ||
| body: PropTypes.string.isRequired, | ||
| timeStamp: PropTypes.string.isRequired, | ||
| id: PropTypes.number.isRequired, | ||
| liked: PropTypes.bool.isRequired, | ||
| onLiked: PropTypes.func, | ||
|
Comment on lines
+35
to
+40
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,39 @@ | ||
| import './ChatLog.css'; | ||
| import ChatEntry from './ChatEntry'; | ||
| import PropTypes from 'prop-types'; | ||
|
|
||
| const ChatLog = ({entries, onLiked}) => { | ||
| const chatEntryComponents = 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 |
||
| return ( | ||
| <ChatEntry | ||
| key={chatEntry.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 |
||
| sender={chatEntry.sender} | ||
| body={chatEntry.body} | ||
| timeStamp={chatEntry.timeStamp} | ||
| id={chatEntry.id} | ||
| liked={chatEntry.liked} | ||
| onLiked={onLiked}> | ||
| </ChatEntry> | ||
| ); | ||
| }); | ||
|
|
||
| return ( | ||
| <div className='chat-log'> | ||
| {chatEntryComponents} | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| ChatLog.propTypes = { | ||
| entries: PropTypes.arrayOf( | ||
| PropTypes.shape({ | ||
| sender: PropTypes.string.isRequired, | ||
| body: PropTypes.string.isRequired, | ||
| timeStamp: PropTypes.string.isRequired, | ||
| id: PropTypes.number.isRequired, | ||
| liked: PropTypes.bool.isRequired, | ||
| })).isRequired, | ||
| onLiked: PropTypes.func, | ||
| }; | ||
|
|
||
| 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.
Prefer to use a lower camelcase symbol here. The import name becomes the variable name used in this file to refer to the data, which here is a list of data, not a class (yes React uses capital letters even though they're functions, but conceptually, they're like a class). An ALL_CAPS name would also be OK.