-
Notifications
You must be signed in to change notification settings - Fork 35
Possum: Jesica G #31
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?
Possum: Jesica G #31
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,17 +1,54 @@ | ||||||
| import './App.css'; | ||||||
| import messagesData from './data/messages.json'; | ||||||
| import ChatLog from './components/ChatLog'; | ||||||
| import { useState } from 'react'; | ||||||
|
|
||||||
| const App = () => { | ||||||
| return ( | ||||||
| <div id="App"> | ||||||
| <header> | ||||||
| <h1>Application title</h1> | ||||||
| </header> | ||||||
| <main> | ||||||
| {/* Wave 01: Render one ChatEntry component | ||||||
| Wave 02: Render ChatLog component */} | ||||||
| </main> | ||||||
| </div> | ||||||
| const toggleLikes = entry => { | ||||||
| return {...entry, liked: !entry.liked}; | ||||||
| }; | ||||||
|
|
||||||
| const countTotalLikes = messages => { | ||||||
| let total = 0; | ||||||
| for ( const entry of 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. Nitpick: issues with whitespacing here
Suggested change
|
||||||
| if (entry.liked) { | ||||||
| total += 1; | ||||||
| } | ||||||
|
Comment on lines
+13
to
+15
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. Using const totalLikes = entries.reduce((total, chatEntry) => {
return chatEntry.isLiked ? total + 1 : total;
}, 0); |
||||||
| } | ||||||
| return total; | ||||||
| }; | ||||||
|
|
||||||
| function App() { | ||||||
| const[messages, setMessages] = useState(messagesData); | ||||||
|
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
|
||||||
|
|
||||||
| const handleLikes = id => { | ||||||
| setMessages(prevMessages=>{ | ||||||
| return prevMessages.map(entry => { | ||||||
|
Comment on lines
+24
to
+25
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){ | ||||||
|
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'll want to fix up code style issues while you're writing code and use a linter to check for style issues before submitting code for review.
Suggested change
|
||||||
| return toggleLikes(entry); | ||||||
| } else { | ||||||
| return entry; | ||||||
| } | ||||||
| }); | ||||||
| }); | ||||||
|
|
||||||
| }; | ||||||
|
|
||||||
| const totalLikes = countTotalLikes(messages); | ||||||
| return ( | ||||||
| <div id="App"> | ||||||
| <header> | ||||||
| <h1>Chat between Vladimir and Estragon</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. Can you think of a way to dynamically get the senders' names without hardcoding "Vladimir" and "Estragon" here? |
||||||
| <h2>{`${totalLikes} ❤️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. You can directly invoke
Suggested change
|
||||||
| </header> | ||||||
| <main> | ||||||
| <ChatLog entries={messages} | ||||||
| onLikeEntry={handleLikes} | ||||||
| /> | ||||||
| </main> | ||||||
| </div> | ||||||
| ); | ||||||
| }; | ||||||
|
|
||||||
|
|
||||||
| export default App; | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,21 +1,32 @@ | ||||||
| import './ChatEntry.css'; | ||||||
| import PropTypes from 'prop-types'; | ||||||
| import TimeStamp from './TimeStamp'; | ||||||
| import { DateTime } from 'luxon'; | ||||||
|
|
||||||
| const ChatEntry = () => { | ||||||
| const ChatEntry = ({id,sender,body,timeStamp,onLikes, 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. 👀 Nitpick: the spacing is inconsistent here because you're missing whitespaces in several places.
Suggested change
|
||||||
| const buttonClass = liked ? 'like__item__toggle--liked' : ''; | ||||||
| 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> | ||||||
| </section> | ||||||
| </replace-with-relevant-semantic-element> | ||||||
| <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. 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. |
||||||
| <h2 className="entry-name">{sender}</h2> | ||||||
| <div className="entry-bubble"> | ||||||
| <p className="entry-body">{body}</p> | ||||||
| <p className="entry-time"><TimeStamp time={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. Nice work using the provided |
||||||
| <button className={`like like__item__toggle ${buttonClass}`} onClick= {() => {onLikes(id); | ||||||
| }}> | ||||||
| {liked ? '❤️' : '🤍'} | ||||||
| </button> | ||||||
| </div> | ||||||
| </section> | ||||||
| ); | ||||||
| }; | ||||||
|
|
||||||
| 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, | ||||||
| onLikes:PropTypes.func, | ||||||
| }; | ||||||
|
|
||||||
| export default ChatEntry; | ||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,37 @@ | ||||||
| import './ChatLog.css'; | ||||||
| import ChatEntry from './ChatEntry'; | ||||||
| import PropTypes from 'prop-types'; | ||||||
|
|
||||||
| const ChatLog =({entries, onLikeEntry }) => { | ||||||
|
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: issue with whitespacing
Suggested change
|
||||||
| const ChatEntryLogs = entries.map(entry=> { | ||||||
|
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
|
||||||
| 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. 👍 The |
||||||
| id={entry.id} | ||||||
| sender= {entry.sender} | ||||||
| body={entry.body} | ||||||
| timeStamp={entry.timeStamp} | ||||||
| liked={entry.liked} | ||||||
| onLikes={onLikeEntry} | ||||||
|
|
||||||
|
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: Remove unnecessary blank line
Suggested change
|
||||||
| /> | ||||||
| ); | ||||||
| }); | ||||||
| return( | ||||||
| <div className="chat-log">{ChatEntryLogs} </div> | ||||||
| //rename tag | ||||||
| ); | ||||||
| }; | ||||||
|
|
||||||
| 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, | ||||||
|
|
||||||
| }; | ||||||
| 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.
A more descriptive name for this function might be
getTotalLikeswhich would convey to others that this function returns a total count of the likes.