Skip to content

Kunzite - Cindy V. #94

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.

Sign up for GitHub

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

Open
civi10 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Kunzite - Cindy V. #94

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e0a5fb7
Waves completed
civi10 Jun 23, 2023
a8d60e7
added Liked prop to ChatEntry and ChatLog, created new State, updated…
civi10 Jun 30, 2023
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
32 changes: 28 additions & 4 deletions src/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,43 @@
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 [totalLikes, setTotalLikes] = useState(0);
const [messages, setMessages] = useState(chatMessages);

const handleLikeCount = (id) => {
const copyMessages = [...messages];
let newLiked;
for (let i = 0; i < copyMessages.length; i++){
if (copyMessages[i].id === id){
const oneMessage = {...copyMessages[i]}
oneMessage.liked = ! oneMessage.liked;
copyMessages[i] = oneMessage;
newLiked = oneMessage.liked;
}
}
Comment on lines +10 to +20
Copy link

@anselrognlie anselrognlie Jul 3, 2023

Choose a reason for hiding this comment

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

If we take the approach of calculating the total liked count from the message list directly, then this logic can be simplified to focus only on updating the liked status of the clicked message. The code as written does this explicitly by duplicating the message list, locating the message that was clicked, duplicating it, updating its liked status, replacing the message in the copied list with the updated message, and setting the message.

We can write this more compactly, and more clearly (at least once we're comfortable with the map function) as follows:

  const handleLikeCount = (id) => {
    setMessage(messages => (  // the current state value will be passed in to our function
      // map will make our new outer list (react needs to see a new list to redraw)
      messages.map(message => (
        // we can avoid using indexing since map passes each value one by one
        // whatever we return will end up at the "current" slot in the new array
        if (message.id === id) {
          // this is the message that was clicked, so make a copy with the liked flipped
          return {...message, liked: !message.liked};
        } else {
          // this message didn't change (it wasn't clicked), so we can return it as is
          return message;
        }
      ));
    ));
  };

Note the use of the callback style of setMessages, which is more robust to asynchronous updates since our code is given the most recent value of the piece of state, which may have diverged from the version of the data that was current at the time of the render that was captured in our callback. Since there's no async code in ChatLog, it's not necessary, but it's useful to get in the habit of using it.

This code may look longer, but it's primarily due to the explanatory comments.



setMessages(copyMessages);
setTotalLikes((prevLikes) => (newLiked ? prevLikes + 1 : prevLikes - 1));
Copy link

@anselrognlie anselrognlie Jul 3, 2023

Choose a reason for hiding this comment

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

Note that we can calculate the total number of liked counts from the current state of the list of messages on each render, rather than managing a separate piece of state for it. It may seem more efficient to track a piece of state for the count, rather than needing to iterate over the list of messages to calculate it on every render, but keep in mind that React will need to iterate over the entire messages array to generate our list of ChatEntry components anyway, so this effectively adds only a small additional sum to work we already need to do. Remember that from a complexity standpoint, it makes no difference whether we iterate once and perform two operations in the loop, or iterate twice and perform a single operation (so looping and summing, then later looping and building ChatEntry components have the same cost, and the summing is negligible). It's true that if there were other state changes that could occur and this forced our app to re-render the entire message list each time, we could start to see performance problems, but React provides ways to help alleviate this. So as a starting point, it's generally best to calculate any values that we can from the fewest possible pieces of state.

In order to do so, we could write a helper function to iterate through the message data and count the number of liked messages, or we can leverage built-in reduce method to help as follows.

  // this code can be placed in the main body of the component, anywhere before the return
  // it creates a local variable that holds the calculated count of liked messages
  // adding a bool to a number treats true as 1 and 0 as false
  const totalLikes = messages.reduce((totalLikes, message) => (totalLikes + message.liked), 0);

};



return (
<div id="App">
<header>
<h1>Application title</h1>
<h1>Vladimir and Estragon</h1>
<p>{totalLikes} {totalLikes === 1 ? '❤️' : '❤️s'}</p>
Copy link

@anselrognlie anselrognlie Jun 27, 2023

Choose a reason for hiding this comment

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

👍 Nice handling of the singular and plural.

</header>
<main>
{/* Wave 01: Render one ChatEntry component
Wave 02: Render ChatLog component */}
<ChatLog entries={messages} onLikeChange={handleLikeCount} />
</main>
</div>
);
};

export default App;

31 changes: 24 additions & 7 deletions src/components/ChatEntry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,39 @@
import React from 'react';
import React, { useState } from 'react';
import './ChatEntry.css';
import PropTypes from 'prop-types';
import TimeStamp from './TimeStamp';

const ChatEntry = ({ id, sender, body, timeStamp, onLikeChange, liked }) => {

const handleLike = () => {

onLikeChange(id);
};

const ChatEntry = (props) => {
return (
<div className="chat-entry local">
<h2 className="entry-name">Replace with name of sender</h2>
<h2 className="entry-name">{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>{body}</p>
<p className="entry-time">
<TimeStamp time={timeStamp} />
Copy link

@anselrognlie anselrognlie Jun 27, 2023

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.

</p>
<button className="like" onClick={handleLike}>
{liked ? '❤️' : '🤍'}
Copy link

@anselrognlie anselrognlie Jun 27, 2023

Choose a reason for hiding this comment

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

👀 Prefer to base this off the liked value available as part of the message data which we can pass in as part of the props (currently, this is using a local piece of state).

</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,
onLikeChange: PropTypes.func.isRequired,
Copy link

@anselrognlie anselrognlie Jun 27, 2023

Choose a reason for hiding this comment

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

Because this proptype is marked isRequired, we're getting warnings in the tests, since the test author didn't know what you were going to call this property, and so it's absent from the props passed in during the tests. To avoid the warnings, we can leave off isRequired.

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 (onLikeChange) {
        onLikeChange(id);  // (id is currently not being passed to this component, but should be)
    }

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

Copy link

@anselrognlie anselrognlie Jun 27, 2023

Choose a reason for hiding this comment

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

👀 We should pass the liked value of the message into the ChatEntry (both in the destructuring and the prop types here). Ideally, this component shouldn't need to know how to track and update the liked status itself. Rather, it should display the status to match whatever liked value it receives through props.

liked: PropTypes.bool.isRequired
};

export default ChatEntry;

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

const ChatLog = ({ entries, onLikeChange }) => {
return (
<div className="chat-log">
{entries.map((entry) => (
Copy link

@anselrognlie anselrognlie Jun 27, 2023

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.

<ChatEntry
key={entry.id}
id={entry.id}
sender={entry.sender}
body={entry.body}
timeStamp={entry.timeStamp}
onLikeChange={onLikeChange}
Copy link

@anselrognlie anselrognlie Jun 27, 2023

Choose a reason for hiding this comment

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

👀 We should pass the liked status for the current message down into the ChatEntry.

liked={entry.liked}
/>
))}
</div>
);
};

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
})
Copy link

@anselrognlie anselrognlie Jun 27, 2023

Choose a reason for hiding this comment

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

👀 The shape content should also include the message's liked status.

).isRequired,
onLikeChange: PropTypes.func.isRequired,
};

export default ChatLog;