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

Kunzite - Barbara #96

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 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
42 changes: 36 additions & 6 deletions src/App.js
Original file line number Diff line number Diff line change
@@ -1,19 +1,49 @@
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 [messages, setMessages] = useState(chatMessages);
const [likeCount, setLikeCount] = useState(0);

Choose a reason for hiding this comment

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

👀 The number of liked messages can be determined from inspecting the current list of messages. Prefer to avoid introducing additional state when the existing state provides the information we need, and without the risk of state getting out of synch.

We can use a few approaches to get the liked count from the message data. The most javascript-esque way is probably to use reduce, like

  const likeCount = messages.reduce((total, message) => {
    return total + message.liked;  // true will act as 1, false as 0
  }, 0)

Another approach (which makes a copy of the list, but which doesn't really matter since we're mapping this data later anyway) would be

  const likeCount = messages.filter(message => message.liked).length;

Either approach does need to iterate over the message list, which we can avoid with the stand alone count approach. However, since we map over the list later anyway, the additional cost of the iteration here is negligible. If we did end up with other interactions that caused a re-render and didn't want to perform the calculation unnecessarily, there are ways to memoize results in React that can be used to avoid unnecessary calculations. But we should start by deriving values from existing state when possible, and only look for optimizations if the rendering performance is adversely impacted.


const handleLikeUpdate = (id) => {
setMessages(messages => {

Choose a reason for hiding this comment

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

👍 The next state of messages depends on the previous state, so using the callback style of the setState method is a good call.

return messages.map(message => {
if (id === message.id) {
return {
...message,
liked: !message.liked,
}
Comment on lines +14 to +17

Choose a reason for hiding this comment

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

👍 Flipping the liked value here keeps that logic closer to where the state is actually maintained, and with code that needs to "know" what it means to toggle the liked status of a message. Note that we could even move this logic further out of the component so that React doesn't need to know the business rules of these changes. We mostly want to keep React focused on the UI, while flipping the message liked value isn't really a UI concern. React could use a helper to perform this operation without knowing the details of its implementation.

} else {
return message;
}
});
});
};

const updateLikeCount = ((liked) => {
if (liked) {
setLikeCount(() => likeCount + 1);
} else{
setLikeCount(() => likeCount - 1);
}
});
Comment on lines +25 to +31

Choose a reason for hiding this comment

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

👀 If we go with one of the approaches to calculate the liked count from the message data, then we don't need this function to manage the liked count state.


return (
<div id="App">
<header>
<h1>Application title</h1>
<h1>Barbara's Chatlog</h1>
</header>
<main>
{/* Wave 01: Render one ChatEntry component
Wave 02: Render ChatLog component */}
<h2>{likeCount} ❤️s</h2>
<ChatLog
entries={messages}
onHandleLikeUpdate={handleLikeUpdate}
onUpdateLikeCount={updateLikeCount}

Choose a reason for hiding this comment

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

👀 Removing the liked count state logic would also free us from needing to pass down this additional callback.

/>
</main>
</div>
);
};

export default App;
export default App;
46 changes: 37 additions & 9 deletions src/components/ChatEntry.js
Original file line number Diff line number Diff line change
@@ -1,22 +1,50 @@
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 [heart, setHeart] = useState('🤍');

Choose a reason for hiding this comment

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

👀 The value for heart doesn't need to be (and shouldn't be) stored in a local piece of state. One of the values passed in the props is the liked value, and that value (coming all the way from the state stored in App) gets updated when the like button is clicked. We can calculate which heart emoji to display as follows:

  const heart = props.liked ? '❤️' : '🤍';

We don't need a full piece of state here, since the liked value is tracked in state above this component. We should avoid duplicating the value here in state. Instead, we can just calculate a local variable (it will be calculated on each render), or even embed the emoji calculation in the button markup itself (though I do like storing it in a local variable)!

        <button className="like" onClick={onLikeButtonClick}>{props.liked ? '❤️' : '🤍'}</button>


const onLikeButtonClick = () => {
const likedMessage = {

Choose a reason for hiding this comment

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

👀 This style of creating the new object to go in our list is how Learn showed working with the student attendance example. However, it turns out we don't need to do this when we send the id of the record we want to report has changed as you did with your onHandleLikeUpdate call. The id passed there can come directly from props, as

    props.onHandleLikeUpdate(props.id);

id: props.id,
sender: props.sender,
body: props.body,
timeStamp: props.timeStamp,
liked: !props.liked,
};
props.onHandleLikeUpdate(likedMessage.id);
props.onUpdateLikeCount(likedMessage.liked);

Choose a reason for hiding this comment

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

👀 While making a separate call to update the count of liked messages works, updating the count could also be done in the same function that handles the like count flipping (it can determine whether the number of likes should go up or down). This would help reduce the likelihood of the different pieces of state getting out of sync. But an even more effective mechanism is to eliminate the extra piece of state entirely, and to calculate the number of liked values on render (see additional comments in App.js).


if (heart === '🤍') {
return setHeart('❤️');
} else {
return setHeart('🤍');
}
Comment on lines +19 to +23

Choose a reason for hiding this comment

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

If we eliminate 👀 If we eliminate the local piece of state, we can also eliminate these calls. Note that the setState functions don't return anything, so writing return setHeart('❤️'); is unexpected. First, since the set function returns nothing, this is always explicitly returning undefined. Second, since this is an event handler function (it's handling when the like is clicked), there's not really anywhere for the return value to go, since React will ignore anything the event handler returns.

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

Choose a reason for hiding this comment

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

👍 Nice use of the TimeStamp component.

time={props.timeStamp}
/>
</p>
<button className="like" onClick={onLikeButtonClick}>{heart}</button>
</section>
</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,
onHandleLikeUpdate: PropTypes.func,
onUpdateLikeCount: PropTypes.func,
Comment on lines +46 to +47

Choose a reason for hiding this comment

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

Leaving these not marked isRequired avoids a bunch of warnings when running the tests (the tests don't know what you're going to call the event handlers). Ideally, we'd also include code to ensure that any "optional" values are actually present before trying to use them. For example, in the like click handler we could do something like

    if (props.onHandleLikeUpdate) {
        props.onHandleLikeUpdate(likedMessage.id);
    }

which would only try to call the function if it's not falsy (and undefined is falsy).

};

export default ChatEntry;
export default ChatEntry;
39 changes: 39 additions & 0 deletions src/components/ChatLog.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import React from 'react';
import PropTypes from 'prop-types';
import './ChatLog.css';
import ChatEntry from './ChatEntry';
const ChatLog = (props) => {
return (
<section>
{props.entries.map((message) => {

Choose a reason for hiding this comment

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

👍 Nice inline map to generate ChatEntry components from the message data.

return (
<ChatEntry
key={message.id}

Choose a reason for hiding this comment

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

👍

id={message.id}
sender={message.sender}
body={message.body}
timeStamp={message.timeStamp}
liked={message.liked}
onHandleLikeUpdate={props.onHandleLikeUpdate}
onUpdateLikeCount={props.onUpdateLikeCount}
/>
)})}
</section>
);
}
ChatLog.propTypes = {
entries: PropTypes.arrayOf(
PropTypes.shape({

Choose a reason for hiding this comment

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

👍 Nice use of shape to describe the data the values within the array should have.

id: PropTypes.number.isRequired,
sender: PropTypes.string.isRequired,
body: PropTypes.string.isRequired,
timeStamp: PropTypes.string.isRequired,
liked: PropTypes.bool.isRequired
})
)
.isRequired,
onHandleLikeUpdate: PropTypes.func,
onUpdateLikeCount: PropTypes.func
};

export default ChatLog;