-
Notifications
You must be signed in to change notification settings - Fork 117
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
Kunzite - Barbara #96
base: main
Are you sure you want to change the base?
Changes from 7 commits
c7a2cba
09f4c9a
1cdc21a
d109c01
f2281ae
8330b02
14c6335
499a84e
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,19 +1,49 @@ | ||
import React from 'react'; | ||
import React, { useState } from 'react'; | ||
import './App.css'; | ||
import chatMessages from './data/messages.json'; | ||
import ChatLog from './components/ChatLog'; | ||
|
||
const App = () => { | ||
const [messages, setMessages] = useState(chatMessages); | ||
const [likeCount, setLikeCount] = useState(0); | ||
|
||
const handleLikeUpdate = (id) => { | ||
setMessages(messages => { | ||
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 next state of |
||
return messages.map(message => { | ||
if (id === message.id) { | ||
return { | ||
...message, | ||
liked: !message.liked, | ||
} | ||
Comment on lines
+14
to
+17
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. 👍 Flipping the |
||
} else { | ||
return message; | ||
} | ||
}); | ||
}); | ||
}; | ||
|
||
const updateLikeCount = ((liked) => { | ||
if (liked) { | ||
setLikeCount(() => likeCount + 1); | ||
} else{ | ||
setLikeCount(() => likeCount - 1); | ||
} | ||
}); | ||
Comment on lines
+25
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. 👀 If we go with one of the approaches to calculate the liked count from the message data, then we don't need this function to manage the liked count state. |
||
|
||
return ( | ||
<div id="App"> | ||
<header> | ||
<h1>Application title</h1> | ||
<h1>Barbara's Chatlog</h1> | ||
</header> | ||
<main> | ||
{/* Wave 01: Render one ChatEntry component | ||
Wave 02: Render ChatLog component */} | ||
<h2>{likeCount} ❤️s</h2> | ||
<ChatLog | ||
entries={messages} | ||
onHandleLikeUpdate={handleLikeUpdate} | ||
onUpdateLikeCount={updateLikeCount} | ||
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. 👀 Removing the liked count state logic would also free us from needing to pass down this additional callback. |
||
/> | ||
</main> | ||
</div> | ||
); | ||
}; | ||
|
||
export default App; | ||
export default App; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,22 +1,50 @@ | ||
import React from 'react'; | ||
import React, { useState } from 'react'; | ||
import './ChatEntry.css'; | ||
import PropTypes from 'prop-types'; | ||
|
||
import TimeStamp from './TimeStamp'; | ||
const ChatEntry = (props) => { | ||
const [heart, setHeart] = useState('🤍'); | ||
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 value for const heart = props.liked ? '❤️' : '🤍'; We don't need a full piece of state here, since the liked value is tracked in state above this component. We should avoid duplicating the value here in state. Instead, we can just calculate a local variable (it will be calculated on each render), or even embed the emoji calculation in the button markup itself (though I do like storing it in a local variable)! <button className="like" onClick={onLikeButtonClick}>{props.liked ? '❤️' : '🤍'}</button> |
||
|
||
const onLikeButtonClick = () => { | ||
const likedMessage = { | ||
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. 👀 This style of creating the new object to go in our list is how Learn showed working with the student attendance example. However, it turns out we don't need to do this when we send the id of the record we want to report has changed as you did with your props.onHandleLikeUpdate(props.id); |
||
id: props.id, | ||
sender: props.sender, | ||
body: props.body, | ||
timeStamp: props.timeStamp, | ||
liked: !props.liked, | ||
}; | ||
props.onHandleLikeUpdate(likedMessage.id); | ||
props.onUpdateLikeCount(likedMessage.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. 👀 While making a separate call to update the count of liked messages works, updating the count could also be done in the same function that handles the like count flipping (it can determine whether the number of likes should go up or down). This would help reduce the likelihood of the different pieces of state getting out of sync. But an even more effective mechanism is to eliminate the extra piece of state entirely, and to calculate the number of liked values on render (see additional comments in App.js). |
||
|
||
if (heart === '🤍') { | ||
return setHeart('❤️'); | ||
} else { | ||
return setHeart('🤍'); | ||
} | ||
Comment on lines
+19
to
+23
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. If we eliminate 👀 If we eliminate the local piece of state, we can also eliminate these calls. Note that the |
||
}; | ||
return ( | ||
<div className="chat-entry local"> | ||
<h2 className="entry-name">Replace with name of sender</h2> | ||
<h2 className="entry-name">{props.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>{props.body}</p> | ||
<p className="entry-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 TimeStamp component. |
||
time={props.timeStamp} | ||
/> | ||
</p> | ||
<button className="like" onClick={onLikeButtonClick}>{heart}</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, | ||
onHandleLikeUpdate: PropTypes.func, | ||
onUpdateLikeCount: PropTypes.func, | ||
Comment on lines
+46
to
+47
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. Leaving these not marked if (props.onHandleLikeUpdate) {
props.onHandleLikeUpdate(likedMessage.id);
} which would only try to call the function if it's not falsy (and |
||
}; | ||
|
||
export default ChatEntry; | ||
export default ChatEntry; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
import React from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import './ChatLog.css'; | ||
import ChatEntry from './ChatEntry'; | ||
const ChatLog = (props) => { | ||
return ( | ||
<section> | ||
{props.entries.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 inline |
||
return ( | ||
<ChatEntry | ||
key={message.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. 👍 |
||
id={message.id} | ||
sender={message.sender} | ||
body={message.body} | ||
timeStamp={message.timeStamp} | ||
liked={message.liked} | ||
onHandleLikeUpdate={props.onHandleLikeUpdate} | ||
onUpdateLikeCount={props.onUpdateLikeCount} | ||
/> | ||
)})} | ||
</section> | ||
); | ||
} | ||
ChatLog.propTypes = { | ||
entries: PropTypes.arrayOf( | ||
PropTypes.shape({ | ||
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 |
||
id: PropTypes.number.isRequired, | ||
sender: PropTypes.string.isRequired, | ||
body: PropTypes.string.isRequired, | ||
timeStamp: PropTypes.string.isRequired, | ||
liked: PropTypes.bool.isRequired | ||
}) | ||
) | ||
.isRequired, | ||
onHandleLikeUpdate: PropTypes.func, | ||
onUpdateLikeCount: 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.
👀 The number of liked messages can be determined from inspecting the current list of messages. Prefer to avoid introducing additional state when the existing state provides the information we need, and without the risk of state getting out of synch.
We can use a few approaches to get the liked count from the message data. The most javascript-esque way is probably to use
reduce
, likeAnother approach (which makes a copy of the list, but which doesn't really matter since we're
map
ping this data later anyway) would beEither approach does need to iterate over the message list, which we can avoid with the stand alone count approach. However, since we map over the list later anyway, the additional cost of the iteration here is negligible. If we did end up with other interactions that caused a re-render and didn't want to perform the calculation unnecessarily, there are ways to memoize results in React that can be used to avoid unnecessary calculations. But we should start by deriving values from existing state when possible, and only look for optimizations if the rendering performance is adversely impacted.