Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions src/App.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,30 @@
import React from 'react';
import React, { useState } from 'react';
import './App.css';
import chatMessages from './data/messages.json';
import ChatEntry from './components/ChatEntry.js';
import ChatLog from './components/ChatLog.js';


Comment on lines +6 to +7

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor style: while this is more blank lines than I would normally use, there's nothing wrong with using more. I recommend, though, that you pick a consistent number of lines between elements. For example, there are three lines here between the imports and the code, but in ChatEntry there are four.

But of course, having some extra blank lines is a nanoscopic issue...


const App = () => {
const [likesCount, setLikesCount] = useState(0);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this version of state rather cleverly avoids having a state variable of all the chat entries. There is a slight downside, though...

Right now all the entries in messages.json have liked set to false, but if we were to imagine that rather taking this from static data we would get this data from an API, it would be possible for some of the entries to have already been liked and thus have a value of true for liked.

However, with this code, those likes would not appear in this number, and in fact, if you could unlike the message, the number would go negative. (In this case you can't "unlike" that message because the true value just gets ignored as I'll explain in a comment in ChatEntry.)

Of course, there's a simple solution to that, and that's to count the likes before making your call to useState, but at the same time, if we were taking in a list of messages from an API, we probably would have to actually maintain the chat entries using useState.

And in that case, rather than having two state variables, one for the entries and one for the count of likes, it would likely be better to only maintain the one for the entries as the number of likes can be determined from the entries.

Having redundant state like that is a common source for bugs as the state that should stay in sync falls out of sync. Not necessarily enough to be a bug, but often what a reviewer would call a "code smell".

But we're well in hypothetical world at this point, just wanted to point that out. I think for our case your implementation is fine!

const updateLikes = (isLike) => {
if (isLike) {
setLikesCount(likesCount + 1);
} else {
setLikesCount(likesCount - 1)
}
}


return (
<div id="App">
<header>
<h1>Application title</h1>
<h2>{likesCount} ❤️s</h2>
</header>
<main>
{/* Wave 01: Render one ChatEntry component
Wave 02: Render ChatLog component */}
<ChatLog entries={chatMessages} updateLikes={updateLikes}/>
</main>
</div>
);
Expand Down
32 changes: 27 additions & 5 deletions src/components/ChatEntry.js
Original file line number Diff line number Diff line change
@@ -1,22 +1,44 @@
import React from 'react';
import './ChatEntry.css';
import PropTypes from 'prop-types';
import TimeStamp from './TimeStamp.js';
import { useState } from 'react';




const ChatEntry = (props) => {
const [likeButton, setLikeButton] = useState('🤍')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned, since the like button setting gets set here to be the empty heart regardless of the actual value in props, if one of the messages had already been liked, then it would still have the empty heart.

But like I said above in App, this doesn't really impact our case, but would have the same issues I outlined if it did.

const toggleClicked = () => {
if (likeButton === '🤍' ) {
setLikeButton('❤️')
props.updateLikes(true)
} else {
setLikeButton('🤍');
props.updateLikes(false)
}

}



return (
<div className="chat-entry local">
<h2 className="entry-name">Replace with name of sender</h2>
<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>
<button className="like" onClick= {toggleClicked}>{likeButton}</button>
</section>
</div>
);
};

// {props.updateLikes}
ChatEntry.propTypes = {
//Fill with correct proptypes
sender: PropTypes.string.isRequired,
body: PropTypes.string.isRequired,
timeStamp: PropTypes.string.isRequired,
Comment on lines +39 to +41

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make sure to include updateLikes in these PropTypes just since it is a large part of making this code work, so leaning on PropTypes to catch errors for you is worth the extra typing. (I guess this is what you were planning with the comment on line 37, but I'm still going to point it out.)

I would even go further and mark updateLikes as .isRequired, though this has the downside of making our test fail due to how we (the instructors) wrote the test data. So fair enough if you did not mark it .isRequired.

};

export default ChatEntry;
29 changes: 29 additions & 0 deletions src/components/ChatLog.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import React from 'react';
import ChatEntry from './ChatEntry';



const ChatLog = (props) => {
const entries = props.entries;

return (
<div className= 'chat-log'>
{
entries.map((entry,id) => (
<ChatEntry
id={entry.id}
sender={entry.sender}
body={entry.body}
timeStamp={entry.timeStamp}
liked={entry.liked}
updateLikes={props.updateLikes}
key={id}
/>
)
)}
</div>
)
};


Comment on lines +27 to +28

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no PropTypes set for this component, which is probably fine as it would be redundant with the ones you do set in ChatEntry. At the same time, it's usually a good habit to use tools like PropTypes to check your expectations, because bugs can crop up in surprising places...

export default ChatLog;