PR for react-chatlog instructor review #44
Conversation
|
FYI @SalmaAnany the link you submitted in Learn wasn't to a valid PR, so I opened this so I can add review comments to it. |
src/App.jsx
Outdated
There was a problem hiding this comment.
App.jsx should have the following (you can reference examples of how we did this in flasky-front-end, review this link here):
- function to calculate the number of likes from each ChatEntry (see lines 83-87 and lines 89, 95 in App.jsx in the link above)
- state that represents all the messages (see line 42 in App.jsx in the link above)
- an event handling function that updates the state when a message is liked or unliked so the page can re-render that is passed down as a prop to
ChatLog.jsxand then down toChatEntry.jsx(see lines 44-55 and line 98 in App.jsx in the link above)
src/components/ChatEntry.jsx
Outdated
| import TimeStamp from './TimeStamp.jsx'; | ||
|
|
||
| const ChatEntry = ({sender,body,timeStamp, liked, numOfLikes, setLikes, isLocal}) => { | ||
| const [like, setLike] = useState(liked); |
There was a problem hiding this comment.
We don't need to track if a ChatEntry has been liked with state here.
If App.jsx knows about all the components (which is why we lift state out of ChatEntry and up to App), then we can get rid of state here.
See how the Cat.jsx component is structured here. We can think of Cat as being equivalent to ChatEntry.
src/components/ChatEntry.jsx
Outdated
| <button className="like" onClick={() =>{ | ||
| if (like === true) { | ||
| setLike(false); | ||
| setLikes(numOfLikes - 1); | ||
| } else { | ||
| setLike(true); | ||
| setLikes(numOfLikes + 1); | ||
| } |
There was a problem hiding this comment.
Instead of onClick being an anonymous function with logic, I'd expect the onClick event handler to invoke a function that is passed down from App.jsx to ChatLog.jsx and finally down to this component here ChatEntry.jsx.
You can find an example of what this looks like at this link, see lines 17-21 and line 35 in Cat.jsx
Petting a cat in flasky-front-end is the equivalent of liking a ChatEntry.
src/components/ChatLog.jsx
Outdated
| {`${numOfLikes} ❤️s`} | ||
| </div> | ||
| { | ||
| entries?.map(i => <ChatEntry key={i.id} body={i.body} sender={i.sender} timeStamp={i.timeStamp} liked={i.liked} numOfLikes={numOfLikes} setLikes={setLikes} |
There was a problem hiding this comment.
This line is challenging to read and violates the JS styleguide that suggests each line is no more than 120 characters long.
Using new lines for each prop being passed would help with readability.
There was a problem hiding this comment.
Prefer a more descriptive name than i. Maybe entry?
src/components/ChatLog.jsx
Outdated
| const [numOfLikes, setLikes] = useState(entries?.filter(i => i.liked === true)?.length); | ||
| return <div> | ||
| <div> | ||
| {`${numOfLikes} ❤️s`} |
There was a problem hiding this comment.
This element should live in App.jsx, at the highest level of the hierarchy of all the components.
You can see an example of how this looks in App.jsx on line 95 here in flasky-front-end
|
|
||
| const App = () => { | ||
| const [chatEntries, setChatEntries] = useState(data); |
| setChatEntries(updatedEntries); | ||
| }; | ||
|
|
||
| const totalLikes = chatEntries.filter((entry) => entry.liked).length; |
There was a problem hiding this comment.
You could also use reduce too:
// adding a bool to a number treats true as 1 and 0 as false
const totalLikes = chatEntries.reduce((totalLikes, entry) => (totalLikes + entry.liked), 0);| entries={chatEntries} | ||
| onToggleLike={toggleLike} |
There was a problem hiding this comment.
Yep, exactly the props that need to get passed down.
| const ChatLog = ({ entries, onToggleLike }) => { | ||
| return ( | ||
| <div> | ||
| {entries.map((entry) => ( | ||
| <ChatEntry | ||
| key={entry.id} | ||
| id={entry.id} | ||
| body={entry.body} | ||
| sender={entry.sender} | ||
| timeStamp={entry.timeStamp} | ||
| liked={entry.liked} | ||
| onToggleLike={onToggleLike} | ||
| isLocal={entry.sender === 'Vladimir'} | ||
| /> | ||
| ))} | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Typically you'll see the result of using map referenced by a variable and using that variable in the JSX, like:
const chatLogs = entries.map((entry) => {
return (
<ChatEntry
key={entry.id}
{...entry}
onLikeToggle={onLikeToggle}
isLocal={entry.sender === 'Vladimir'}
/>
);
});
};
return (
<div>
{chatLogs}
</div>
);| </p> | ||
| <button | ||
| className="like" | ||
| onClick={handleLikeClick} |
No description provided.