Skip to content
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

Tigers - Mica C #114

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
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
14 changes: 12 additions & 2 deletions src/App.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,26 @@
import React from 'react';
import React, { useState } from 'react';
import './App.css';
import chatMessages from './data/messages.json';
import ChatLog from './components/ChatLog';

const App = () => {
const [likeCount, setLikeCount] = useState(0);
const updateLikeCount = (increment) => {
setLikeCount(likeCount + increment);
};
Comment on lines +7 to +10

Choose a reason for hiding this comment

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

I was about to comment that there was no need to have a likeCount state variable, but then I realized that this 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 unliked the message, the number would go negative.

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!

Choose a reason for hiding this comment

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

Interested in seeing what happened, I cloned your code and set one the messages to have a liked of true and to my surprise (because I haven't looked at ChatEntry yet, that message was displayed as not liked. Which, again, would be fine without my changes, but everything I said in this comment would apply to the heart widget in ChatEntry too.

return (
<div id="App">
<header>
<h1>Application title</h1>
<h1>Vladimir and Estragon's Conversation</h1>
<h2>{`${likeCount} ❤️s`}</h2>
</header>
<main>
{/* Wave 01: Render one ChatEntry component
Wave 02: Render ChatLog component */}
<ChatLog
entries={chatMessages}
onUpdateLikeCount={updateLikeCount}
></ChatLog>
</main>
</div>
);
Expand Down
5 changes: 4 additions & 1 deletion src/components/ChatEntry.css
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ button {
outline: inherit;
}

button.liked {
}
Comment on lines +11 to +12

Choose a reason for hiding this comment

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

Looks like nothing got added to this rule. It happens, but it's usually worth deleting these before checking in.


.chat-entry {
margin: 1rem;
}
Expand Down Expand Up @@ -97,4 +100,4 @@ button {

.chat-entry.remote .entry-bubble:hover::before {
background-color: #a9f6f6;
}
}
37 changes: 30 additions & 7 deletions src/components/ChatEntry.js
Original file line number Diff line number Diff line change
@@ -1,22 +1,45 @@
import React from 'react';
import React, { useState } from 'react';
import './ChatEntry.css';
import PropTypes from 'prop-types';
import TimeStamp from './TimeStamp';

const ChatEntry = (props) => {
const source = props.sender === 'Vladimir' ? 'local' : 'remote';

Choose a reason for hiding this comment

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

Good use of the ternary operator! Also good way to distinguish the remote user from the local user!

Further, I think within the hierarchy this is exactly correct place for this check. It wouldn't make sense higher up.

const [liked, setLiked] = useState(props.liked);
const toggleLikeButton = () => {
const heart = document.getElementById(props.id);
setLiked(!liked);
if (liked) {
heart.textContent = '🤍';
props.onUpdate(-1);
} else {
heart.textContent = '❤️';
props.onUpdate(1);
}
setLiked(!liked);
};
Comment on lines +8 to +20

Choose a reason for hiding this comment

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

Ah, though looking at this, this is a bit of an awkward fallout from the decision above.

To make sure the heart button updates the setting, you have to use useState here to keep track of the liked status. But I notice that your if statement is "backwards" here, and that's because I figure you realized that setLiked does not update the liked variable immediately. Instead these setter functions that useState returns updates on re-render.

And there's nothing "wrong" per se with the logic being backwards here, but it is something that could be a bit of confusion for sure.

I think the best solution would be that rather than always drawing the empty heart and letting it change when the button is clicked, pick which heart to draw based on props.liked. Then you can simply call setLiked here and it will be updated on re-render to the right heart.

Choose a reason for hiding this comment

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

Also there's an extra call to setLiked, which I presume is a typo, and from testing my clone of your code, you can remove it.

return (
<div className="chat-entry local">
<h2 className="entry-name">Replace with name of sender</h2>
<div className={`chat-entry ${source}`}>
<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}></TimeStamp>
</p>
<button className="like" id={props.id} onClick={toggleLikeButton}>
🤍
</button>
</section>
</div>
);
};

ChatEntry.propTypes = {
//Fill with correct proptypes
id: PropTypes.number,
sender: PropTypes.string,
body: PropTypes.string,
timeStamp: PropTypes.string,
liked: PropTypes.bool,
Comment on lines +38 to +42

Choose a reason for hiding this comment

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

If these were .isRequired in ChatLog, they probably should be here too. Now, that being said I know that causes a few test failures, but that's due to our test data not setting these. Either way, completely understand why you may not have set it here, given that.

};

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

const ChatLog = (props) => {
const chatEntryComponents = props.entries.map((entry) => {
return (
<ChatEntry
key={entry.id}
id={entry.id}
sender={entry.sender}
body={entry.body}
timeStamp={entry.timeStamp}
liked={entry.liked}
onUpdate={props.onUpdateLikeCount}
/>
);
});
return <ul class-name="chat-log">{chatEntryComponents}</ul>;
};

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,
};

export default ChatLog;