-
Notifications
You must be signed in to change notification settings - Fork 35
Anaiah -Possom C24 #32
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
a04511f
53123c5
127380e
bba269c
c1c9286
fe98f0c
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,31 @@ | ||
| import './App.css'; | ||
| import ChatLog from './components/ChatLog'; | ||
| import MessageData from './data/messages.json'; | ||
| import { useState } from 'react'; | ||
|
|
||
| const App = () => { | ||
| const [messages, setMessages] = useState(MessageData); | ||
|
|
||
| const toggleLike = (id) => { | ||
| setMessages(prevMessages => | ||
| prevMessages.map(message => | ||
| message.id === id | ||
| ? { ...message, liked: !message.liked } | ||
| : message | ||
| ) | ||
| ); | ||
| }; | ||
|
|
||
| const totalLikes = messages.filter(message => message.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! You could also use |
||
|
|
||
| return ( | ||
| <div id="App"> | ||
| <header> | ||
| <h1>Application title</h1> | ||
| <h1>Messenger App</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. ❗️ On line 24, you should have changed the title so that the header rendered "Chat between Vladimir and Estragon" Can you think of a way to dynamically get the senders' names without hardcoding "Vladimir" and "Estragon" here? |
||
| <h2>{totalLikes} ❤️s</h2> | ||
| </header> | ||
| <main> | ||
| {/* Wave 01: Render one ChatEntry component | ||
| Wave 02: Render ChatLog component */} | ||
| <ChatLog entries={messages} onToggleLike={toggleLike}></ChatLog> | ||
| </main> | ||
| </div> | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -97,4 +97,5 @@ button { | |
|
|
||
| .chat-entry.remote .entry-bubble:hover::before { | ||
| background-color: #a9f6f6; | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,21 +1,34 @@ | ||||||
| import './ChatEntry.css'; | ||||||
| import TimeStamp from './TimeStamp'; | ||||||
| import PropTypes from 'prop-types'; | ||||||
|
|
||||||
| const ChatEntry = (props) => { | ||||||
|
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. Destructuring Destructuring props also has some benefits:
Ultimately, you'll adapt what you do depending on what your team does. |
||||||
|
|
||||||
| 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> | ||||||
| <section className="entry-bubble"> | ||||||
| <p>Replace with body of ChatEntry</p> | ||||||
| <p className="entry-time">Replace with TimeStamp component</p> | ||||||
| <button className="like">🤍</button> | ||||||
| <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. 👀 Nitpick: white spacing is off
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. It was an optional enhancement, but how could you determine if a chat was from a 'local' sender vs a 'remote' sender so that the chat bubbles would have different styles applied to it? If you have time to re-visit this project for React.js review, it would be good practice to try applying local vs. remote classes to the components so they render differently depending on who sent the message. |
||||||
| <section 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. I think you could remove this The
Suggested change
|
||||||
| <h2 className="entry-name">{props.sender}</h2> | ||||||
| <section className="entry-bubble"> | ||||||
| <p>{props.body}</p> | ||||||
| <p className="entry-time"> | ||||||
| <TimeStamp time={props.timeStamp}></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 work using the provided |
||||||
| </p> | ||||||
| <button className="like" onClick={props.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. Since a
Suggested change
|
||||||
| {props.liked ? '❤️' : '🤍'} | ||||||
| </button> | ||||||
| </section> | ||||||
| </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, | ||||||
| liked: PropTypes.bool.isRequired, | ||||||
| onToggleLike: PropTypes.func.isRequired, | ||||||
|
|
||||||
| }; | ||||||
|
|
||||||
| export default ChatEntry; | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,22 @@ | ||||||
| import ChatEntry from "./ChatEntry" | ||||||
|
|
||||||
| const ChatLog = (props) => { | ||||||
|
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 prefer props to be destructured, like I mentioned in my comment in |
||||||
| return ( | ||||||
| <section className="chat-log"> | ||||||
| {props.entries.map(entry => ( | ||||||
| <ChatEntry | ||||||
|
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. Another common way you'll see components organized using map is to store the result into a variable we use in the JSX result: const chatEntries = entries.map((entry) => {
return (
<ChatEntry
// your props
/>
);
});
return <section className="chat-log">{chatEntries}</section>; |
||||||
| key={entry.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={entry.sender} | ||||||
| body={entry.body} | ||||||
| timeStamp={entry.timeStamp} | ||||||
| liked={entry.liked} | ||||||
| onToggleLike={() => props.onToggleLike(entry.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. We should directly pass in the event handler function without needing to wrap it in an anonymous function.
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. It should be the job of the |
||||||
| /> | ||||||
| ))} | ||||||
| </section> | ||||||
| ); | ||||||
| }; | ||||||
|
|
||||||
|
|
||||||
| 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.
Nice job using the callback-style for updating the state.