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.

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

Jump to bottom

Kunzite - Danqing Liu #106

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Kunzite - Danqing Liu #106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 24 additions & 0 deletions src/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,27 @@
import React from 'react';
Copy link

Choose a reason for hiding this comment

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

We can add the useState import here

import React, { useState } from 'react';

import './App.css';
import chatMessages from './data/messages.json';
import ChatLog from './components/ChatLog';
import { useState } from 'react';


const App = () => {

const [chatEntries, setChatsLike] = useState(chatMessages)

const updateLikeData = updatedChat =>{
Copy link

Choose a reason for hiding this comment

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

Rather than receiving a new message with it's like status flipped here, rewrite this to accept an id. The logic about how to copy the message and flip the liked status is better housed here, since the state must have all the information necessary to make a new message, whereas in complex systems, we may not pass all of the information need to make a new data element all the way down to a presentation component like ChatEntry.

const chats = chatEntries.map(chat => {
if (chat.id === updatedChat.id) {
return updatedChat;
}else {
return chat;
}
});

setChatsLike(chats)
Copy link

Choose a reason for hiding this comment

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

👀 Since the next version of the chat data depends on the current version, prefer using the callback style of setChatsLike

    setChatsLike(chats => (
      // logic to use current chats to make a new copy with the desired liked status flipped (the map logic above)
      // return the new chats to update the state
    ))


}

return (
<div id="App">
<header>
Expand All @@ -11,6 +30,11 @@ const App = () => {
<main>
{/* Wave 01: Render one ChatEntry component
Wave 02: Render ChatLog component */}
<ChatLog
entries = {chatEntries}
onUpdateChat = {updateLikeData}
></ChatLog>

</main>
</div>
);
Expand Down
31 changes: 26 additions & 5 deletions src/components/ChatEntry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,22 +1,43 @@
import React from 'react';
import './ChatEntry.css';
import PropTypes from 'prop-types';
import TimeStamp from './TimeStamp'

const ChatEntry = (props) => {


const updateLikeButtonClick = () =>{
const updateChat ={
id : props.id,
sender : props.sender,
body : props.body,
liked : !props.liked,
timeStamp : props.timeStamp,
}
props.onUpdate(updateChat);
Comment on lines +10 to +17
Copy link

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. The id passed there can come directly from props, as

    props.onUpdate(props.id);

Our local component here shouldn't need to know how to toggle the like status. Rather, this component should pass the id of the message whose like button was clicked to the callback, and that logic should be responsible for determining what the new liked status for the message should be. This would help ensure we have a single source of truth (the list of messages), from which we can draw the entire UI.

  // in App.js
  const handleLikeClicked = (id) => {
    // copy the list of message stored in the App state
    // copy the message indicated by the id
    // flip the liked status for the message
    // update the App state tracking the messages
  };

  // later in App.js
  <ChatLog entries={messages} onLikeClicked={handleLikeClicked} />

}

const heartColor = props.liked ? '❤️': '🤍';
Copy link

Choose a reason for hiding this comment

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

👍 Nice calculation of the emoji to use based on the liked prop. We could even do this inline in the JSX, though I do like the clarity it brings to do it on its own and give the result a good name.


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}></TimeStamp></p>
Copy link

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.

Stylistically, prefer to omit the spaces around = in JSX attributes. And JSX prefers using the /> syntax for closing empty components.

        <p className="entry-time"><TimeStamp time={props.timeStamp} /></p>

<button onClick ={updateLikeButtonClick} className="like">{heartColor}</button>
</section>
</div>
);
};

ChatEntry.propTypes = {
//Fill with correct proptypes
id: PropTypes.number.isRequired,
sender: PropTypes.string.isRequired,
body: PropTypes.string.isRequired,
liked: PropTypes.bool.isRequired,
timeStamp: PropTypes.string.isRequired,
onUpdate: PropTypes.func.isRequired
Copy link

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 (onUpdate) {
        onUpdate(id);
    }

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

};

export default ChatEntry;
6 changes: 6 additions & 0 deletions src/components/ChatLog.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,9 @@
margin: auto;
max-width: 50rem;
}

#ChatLog h2 {
font-size: 1.5em;
text-align: center;
display: inline-block;
}
53 changes: 53 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,53 @@
import React from 'react';
import './ChatLog.css';
import ChatEntry from './ChatEntry';
import './ChatLog.css'
Copy link

Choose a reason for hiding this comment

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

👀 Redundant import

import PropTypes from 'prop-types';

const ChatLog = (props) => {
const chatLog = 'chat-log';
let showLike;
console.log(props.entries);
Copy link

Choose a reason for hiding this comment

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

👀 Remove debugging logs from projects. This makes a lot of console noise when running tests (and prints out to the console in the browser).

const chatComponents = props.entries.map((chat, index)=> {
const entriesData = [...props.entries];
const totalLike = [...entriesData].reduce((a,b) => a + (b.liked? 1: 0), 0)
Copy link

Choose a reason for hiding this comment

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

👍 Even though I'd recommend moving this logic, this is still a nice use of reduce to total up the number of liked messages in the chat.

showLike = totalLike === 0? '🤍' : totalLike + ' ❤️s';
Comment on lines +12 to +14
Copy link

Choose a reason for hiding this comment

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

The logic for counting the liked messages would be better placed in App. The mockups showed that the intended location for displying the liked count was as part of the page header rather than being an instrinsic part of the conversation. Yes, we can add heading tags her in the ChatLog, but the main responsibility of the ChatLog is to generate the indivdual ChatEntry components and wireup their events properly.

Note that by having this here in the map callback, that means all the messages will be iterated over for each message being converted into a component, making this a O(n^2) operation. We could at the least move this logic out of the map callback and into the body of the component, but it would still be better located in App.


return (
<li key={index}> <ChatEntry
className = {chatLog}
Copy link

Choose a reason for hiding this comment

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

The chat-log class is more meant to be on an element around all of the ChatEntry instances rather than on each ChatEntry.

id ={chat.id}
sender={chat.sender}
body={chat.body}
liked={chat.liked}
timeStamp={chat.timeStamp}
onUpdate = {props.onUpdateChat}
></ChatEntry>
</li>
);
});

return (
<section>
<h2>
{showLike}
</h2>
<ul>
Copy link

Choose a reason for hiding this comment

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

Not every collection of items must be rendered into a list of some kind. As this represents a conversation, leaving it out of a list would be fine (think of it more like paragraphs in an article than items in a list).

{chatComponents}
</ul>
</section>
)
};

ChatLog.propsTypes ={
chatEntrys: PropTypes.arrayOf(PropTypes.shape({
Copy link

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,
liked: PropTypes.bool.isRequired,
timestamp: PropTypes.string.isRequired
})),
onUpdateChat: PropTypes.func.isRequired
};

export default ChatLog;