-
Notifications
You must be signed in to change notification settings - Fork 35
C24 Crow Tatiana Chatlog #21
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 |
|---|---|---|
| @@ -1,14 +1,30 @@ | ||
| import './App.css'; | ||
| import "./App.css"; | ||
| import { useState } from "react"; | ||
| import ChatLog from "./components/ChatLog"; | ||
| import entriesData from "./data/messages.json"; | ||
|
|
||
| const App = () => { | ||
| const [entries, setEntries] = useState(entriesData); | ||
|
|
||
| const likedCount = entries.reduce((sum, e) => sum + (e.liked ? 1 : 0), 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. Great work, @FluenceMind. Though you will find a number of implementations for this, the 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. Given that this function is not dependent on any piece of data that is locally scoped to our component you could opt to move its definition outside of the component and into, say, its own file. |
||
|
|
||
| function handleToggleLike(entryId) { | ||
| setEntries((prev) => | ||
| prev.map((e) => | ||
| e.id === entryId ? { ...e, liked: !e.liked } : e | ||
|
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. Very succinct, however I would encourage you to use a more descriptive variable name than just |
||
| ) | ||
| ); | ||
| } | ||
|
|
||
| return ( | ||
| <div id="App"> | ||
| <header> | ||
| <h1>Application title</h1> | ||
| <p className="liked-count">{likedCount} ❤️s</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. ✅ An alternative could be to call the function here instead of assigning the value to a variable earlier in the code, especially since the variable is not being leveraged in any other way. |
||
| </header> | ||
|
|
||
| <main> | ||
| {/* Wave 01: Render one ChatEntry component | ||
| Wave 02: Render ChatLog component */} | ||
| <ChatLog entries={entries} onToggleLike={handleToggleLike} /> | ||
|
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. 👍🏿 |
||
| </main> | ||
| </div> | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,21 +1,39 @@ | ||||||||||||||||||||
| import './ChatEntry.css'; | ||||||||||||||||||||
| import PropTypes from "prop-types"; | ||||||||||||||||||||
| import TimeStamp from "./TimeStamp"; | ||||||||||||||||||||
| import "./ChatEntry.css"; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const ChatEntry = ({ id, sender, body, timeStamp, liked = false, 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. Once the de-structured params start getting long, you could change the formatting into the following for readability.
Suggested change
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. Also @FluenceMind, wondering the reasoning for having |
||||||||||||||||||||
| const showLikeButton = | ||||||||||||||||||||
| typeof id === "number" && typeof onToggleLike === "function"; | ||||||||||||||||||||
|
Comment on lines
+6
to
+7
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. @FluenceMind, also wondering the reason for having this logic for deciding if the like button will be shown based off the typing for the mentioned props? |
||||||||||||||||||||
|
|
||||||||||||||||||||
| 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"> | ||||||||||||||||||||
| <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> | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
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.
Suggested change
|
||||||||||||||||||||
| <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. ✅ |
||||||||||||||||||||
| </p> | ||||||||||||||||||||
|
|
||||||||||||||||||||
| {showLikeButton && ( | ||||||||||||||||||||
| <button className="like" onClick={() => onToggleLike(id)}> | ||||||||||||||||||||
| {liked ? "❤️" : "🤍"} | ||||||||||||||||||||
|
Comment on lines
+20
to
+22
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. Agnostic of the reason, love see you implementing this! This is essentially how you have functionality like having a submit button appear only when the form is filled out correctly. |
||||||||||||||||||||
| </button> | ||||||||||||||||||||
| )} | ||||||||||||||||||||
| </section> | ||||||||||||||||||||
| </replace-with-relevant-semantic-element> | ||||||||||||||||||||
| </article> | ||||||||||||||||||||
| ); | ||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| ChatEntry.propTypes = { | ||||||||||||||||||||
| // Fill with correct proptypes | ||||||||||||||||||||
| sender: PropTypes.string.isRequired, | ||||||||||||||||||||
| body: PropTypes.string.isRequired, | ||||||||||||||||||||
| timeStamp: PropTypes.string.isRequired, | ||||||||||||||||||||
| id: PropTypes.number, | ||||||||||||||||||||
| liked: PropTypes.bool, | ||||||||||||||||||||
| onToggleLike: PropTypes.func, | ||||||||||||||||||||
|
Comment on lines
+31
to
+36
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. Great work on these proptypes, remember that they have been sunsetted in React and that will be using other type checking frameworks like Typescript. |
||||||||||||||||||||
| }; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| export default ChatEntry; | ||||||||||||||||||||
| export default ChatEntry; | ||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,48 @@ | ||||||||||||||||||||||||
| import PropTypes from "prop-types"; | ||||||||||||||||||||||||
| import ChatEntry from "./ChatEntry"; | ||||||||||||||||||||||||
| import "./ChatLog.css"; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| function ChatLog({ entries, onToggleLike }) { | ||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||
| <section className="chat-log"> | ||||||||||||||||||||||||
| {entries.map((entry) => ( | ||||||||||||||||||||||||
| <ChatEntry | ||||||||||||||||||||||||
| key={entry.id} | ||||||||||||||||||||||||
| id={entry.id} | ||||||||||||||||||||||||
| sender={entry.sender} | ||||||||||||||||||||||||
| body={entry.body} | ||||||||||||||||||||||||
| timeStamp={entry.timeStamp} | ||||||||||||||||||||||||
| liked={entry.liked} | ||||||||||||||||||||||||
| onToggleLike={onToggleLike} | ||||||||||||||||||||||||
|
Comment on lines
+10
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. Great work, especially on the formatting. To simplify your logic, you could pass in the entire message object yourself. However, you would lose out on the explicitness you have here. |
||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||
| ))} | ||||||||||||||||||||||||
| </section> | ||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| 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, | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| 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, | ||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||
|
Comment on lines
+36
to
+46
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. You have two versions of your propTypes for Chatlog. Is this a mistake, @FluenceMind? The one defined above looks like it is the one you need, but then you override with the declaration/assignment of this one.
Suggested change
|
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| 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.
✅