-
Notifications
You must be signed in to change notification settings - Fork 35
Possum - Esmeralda Arreguin-Martinez #11
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
257a60a
3a3e0d7
66b7f14
56f4225
6734b10
18e4ea8
4deaf79
8a6d615
3ea7e8b
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,39 @@ | ||||||||||||
| import './App.css'; | ||||||||||||
| import ChatLog from './components/ChatLog'; | ||||||||||||
| import entriesData from './data/messages.json'; | ||||||||||||
| import { useState } from 'react'; | ||||||||||||
|
|
||||||||||||
| const countTotalLikes = entriesData => { | ||||||||||||
| return entriesData.reduce((acc, entry) => { | ||||||||||||
| if (!entry.liked) { | ||||||||||||
| return acc; | ||||||||||||
| } | ||||||||||||
| return acc + 1; | ||||||||||||
|
Comment on lines
+8
to
+11
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. A more concise way to write this looks like: |
||||||||||||
| }, 0); | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| const App = () => { | ||||||||||||
| const [entryData, setEntriesData] = useState(entriesData); | ||||||||||||
|
|
||||||||||||
| function handleLikes(id) { | ||||||||||||
| setEntriesData(entryData => { | ||||||||||||
| return entryData.map(entry => { | ||||||||||||
|
Comment on lines
+19
to
+20
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 using the callback-style for updating the state. |
||||||||||||
| if (entry.id === id) { | ||||||||||||
| return { ...entry, liked: !entry.liked}; | ||||||||||||
| } | ||||||||||||
| return entry; | ||||||||||||
| }); | ||||||||||||
| }); | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
| return ( | ||||||||||||
| <div id="App"> | ||||||||||||
| <header> | ||||||||||||
| <h1>Application title</h1> | ||||||||||||
| <h2>{countTotalLikes(entryData)} ❤️s</h2> | ||||||||||||
|
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. ❗️ ^^ above on line 32, 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? |
||||||||||||
| </header> | ||||||||||||
| <main> | ||||||||||||
| {/* Wave 01: Render one ChatEntry component | ||||||||||||
| Wave 02: Render ChatLog component */} | ||||||||||||
| <ChatLog entries={entryData} onLikeEntry={handleLikes}></ChatLog> | ||||||||||||
|
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. Prefer to have opening tag, closing tag, and props all on separate lines to help with readability. React doesn't care how you write the component so the key is to be consistent in your project. Also since
Suggested change
|
||||||||||||
| </main> | ||||||||||||
| </div> | ||||||||||||
| ); | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,21 +1,30 @@ | ||
| 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 chatDisplay = props.sender === 'Vladimir' ? 'local' : '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 job writing logic to determine the class to apply to the component so that the components show up with 'local' vs 'remote' styles. |
||
|
|
||
| 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> | ||
| <div className={`chat-entry ${chatDisplay}`}> | ||
| <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 time={props.timeStamp} /></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. Good work using the provided |
||
| <button className="like" onClick={() => props.onLike(props.id)}>{props.liked ? '❤️' : '🤍'}</button> | ||
| </section> | ||
| </replace-with-relevant-semantic-element> | ||
| </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, | ||
| onLike: PropTypes.func.isRequired | ||
| }; | ||
|
|
||
| export default ChatEntry; | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,43 @@ | ||||||
| import './ChatLog.css'; | ||||||
| import ChatEntry from './ChatEntry'; | ||||||
| import PropTypes from 'prop-types'; | ||||||
|
|
||||||
| 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. Like my comment above in |
||||||
|
|
||||||
| const getChatEntries = (entries) => { | ||||||
| return entries.map((entry) => { | ||||||
| return (<ChatEntry | ||||||
| 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. Nitpick: incorrect white spacing
Suggested change
Take care to check your style using a linter or fix up the issues while you're writing the code so that your work conforms to convention and guidelines. 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 |
||||||
| id={entry.id} | ||||||
| sender={entry.sender} | ||||||
| body={entry.body} | ||||||
| timeStamp={entry.timeStamp} | ||||||
| liked={entry.liked} | ||||||
| onLike={props.onLikeEntry}> | ||||||
| </ChatEntry> | ||||||
| ); | ||||||
| }); | ||||||
| }; | ||||||
|
|
||||||
| return ( | ||||||
| <div className='chat-log'> | ||||||
| {getChatEntries(props.entries)} | ||||||
| </div> | ||||||
| ); | ||||||
|
|
||||||
| }; | ||||||
|
|
||||||
| 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, | ||||||
| onLikeEntry: PropTypes.func.isRequired | ||||||
| }; | ||||||
|
|
||||||
| 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.
❗️👀 Functions that are only used within a specific component, like
countTotalLikesis only used inside theAppcomponent on line 33, should be defined inside the component to make the code more readable because it keeps related logic together.Therefore, this function should be defined in the
Appcomponent (like you do withhandleLikeson line 18).