-
Notifications
You must be signed in to change notification settings - Fork 35
Priscilla Andresen - Possum #34
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
7267278
c06d5ca
a002d43
410a46b
29fa918
b043f92
d889b20
8f626c7
af7e4c3
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -28,6 +28,7 @@ | |||||
|
|
||||||
| #App header section { | ||||||
| background-color: #e0ffff; | ||||||
| padding: 0.2em; | ||||||
| } | ||||||
|
|
||||||
| #App .widget { | ||||||
|
|
@@ -71,4 +72,9 @@ | |||||
|
|
||||||
| .purple { | ||||||
| color: purple | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| h2 { | ||||||
|
||||||
| h2 { | |
| #App .widget h2 { |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,14 +1,75 @@ | ||||||
| import './App.css'; | ||||||
| import {useState} from 'react'; | ||||||
| import messagesJSON from './data/messages.json'; | ||||||
| import ChatLog from './components/ChatLog'; | ||||||
|
|
||||||
|
|
||||||
| const countTotalLikes = (messageData) => { | ||||||
| return messageData.reduce((acc, entry) => acc + entry.liked, 0); | ||||||
|
||||||
| return messageData.reduce((acc, entry) => acc + entry.liked, 0); | |
| return messageData.reduce((acc, entry) => acc + (entry.liked === true ? 1 : 0), 0); |
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 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.
I like that you wrapped your reduce call in a well-named helper function. This is one way that we can make using reduce a little more understandable for other programmers reading our code, since the syntax can be a little confusing.
Since the message data is passed in, we could even relocate this function completely out of this function. This is definitely business logic, not rendering logic. So it can be managed separately from our React components.
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.
👍 Another benefit to using the callback style of setter is that it now no longer depends on anything in the enclosing scope (the current state value is passed in as a parameter), so this whole chunk of logic for finding and updating the message could be moved out to a helper function!
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.
👍 Toggling is the only business logic we have for messages in this small app, but we can imagine if there are other operations, those could all have their own functions and potentially be grouped into a class (or at least be collected in a separate file).
Of course, when the true data representation is actually coming from a backend, we might omit such helpers entirely, and just get the updated record as a JSON response, using it directly to replace the previous entry in the list, freeing the frontend from needing to know anything about how to manage the record at all!
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.
👍 Great way to get a list of the users in the messages. Sets preserve insertion order in JS, so we know that the first encountered sender would appear first in the list. Extracting the senders this way also scales to however many senders could be in this conversation.
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.
Fancy! And it handles the regular rule format of "item, item and item" (personally, I'm an Oxford comma fan, so would prefer "item, item, and item").
While not a major concern for any reasonable conversation group size, keep in mind that each of the new intermediate strings created while reduce iterates will grow longer and longer, tending towards quadratic time with the total length of the names. Instead, we could determine the part of the list that gets joined with ",", and handle those in one join call, then tack on the "and" part. There is still some extra copying, but it's a fixed number, so ternding more towards linear. Alernatively, we could build up a list of all the parts (names as well as separators), then join them with no separator in a single pass. This is the closest approach to a typical "string buffer/string builder" approach.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,4 +97,9 @@ button { | |
|
|
||
| .chat-entry.remote .entry-bubble:hover::before { | ||
| background-color: #a9f6f6; | ||
| } | ||
| } | ||
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
|
Comment on lines
+100
to
+105
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. Try to avoid making stray changes to other files. When reviewing the contents of a commit, if we see a change like this, it would be preferable to revert it rather than committing it. |
||
| 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 = ({ id, sender, body, timeStamp, liked, onLike }) => { | ||
|
|
||
| const handleLike = () => { | ||
| 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. Relaxing the |
||
| }; | ||
|
|
||
| const like = liked ? '❤️' : '🤍'; | ||
|
|
||
| const ChatEntry = () => { | ||
| 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. 👍 |
||
| <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} /></p> | ||
| <button onClick={handleLike} className="like">{like}</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, | ||
| }; | ||
|
|
||
| export default ChatEntry; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| import ChatEntry from './ChatEntry'; | ||
| import './ChatLog.css'; | ||
| import PropTypes from 'prop-types'; | ||
|
|
||
|
|
||
| const ChatLog = ({ entries, onLike }) => { | ||
| const chatEntries = (chatLog) => { | ||
| return chatLog.map((entry) => { | ||
| return ( | ||
| <ChatEntry | ||
| key={entry.id} | ||
| id={entry.id} | ||
| sender={entry.sender} | ||
| body={entry.body} | ||
| timeStamp={entry.timeStamp} | ||
| liked={entry.liked} | ||
| onLike={onLike} | ||
| /> | ||
| ); | ||
| }); | ||
| }; | ||
|
|
||
| return ( | ||
| <div className="chat-log"> | ||
| {chatEntries(entries)} | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| ChatLog.propTypes = { | ||
| entries: PropTypes.arrayOf( | ||
|
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 matches the expected name now. |
||
| PropTypes.shape({ | ||
| id: PropTypes.number.isRequired, | ||
| sender: PropTypes.string.isRequired, | ||
| body: PropTypes.string.isRequired, | ||
| timeStamp: PropTypes.string.isRequired, | ||
| liked: PropTypes.bool.isRequired, | ||
| }) | ||
| ).isRequired, | ||
| onLike: 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 selector uses
.widgetclass but the actual element hasid="widget". CSS class selectors start with.but this should be an id selector#widgetor the HTML should useclassName="widget"instead ofid="widget".