Skip to content

Sphinx - Vlada Rapaport #21

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
vladarap88 wants to merge 4 commits into
base: main
Choose a base branch
from
+81 −8
Open

Sphinx - Vlada Rapaport #21

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f30a089
completed wave 1
vladarap88 Dec 16, 2024
674d76d
completed wave 2
vladarap88 Dec 16, 2024
e5ab60b
completed wave3
vladarap88 Dec 16, 2024
2a0a40e
updated chatlog prop
vladarap88 Dec 16, 2024
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: 22 additions & 2 deletions src/App.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,34 @@
import { useState } from 'react';
import './App.css';
import ChatLog from './components/ChatLog';
import messages from './data/messages.json';

const App = () => {
const [counter, setCounter] = useState(0); // Initial counter set to 0
Copy link

@yangashley yangashley Dec 19, 2024

Choose a reason for hiding this comment

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

Prefer not to have comments in code unless absolutely necessary.

Suggested change
const [counter, setCounter] = useState(0); // Initial counter set to 0
const [counter, setCounter] = useState(0);

Copy link

@yangashley yangashley Dec 19, 2024

Choose a reason for hiding this comment

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

Per the pattern we did during the flasky-front-end livecode and what we hope to see in the task-list-front-end activity, we want state to keep track of ChatEntrys here. Or in flasky-front-end, the state in App.jsx kept track of individual Cats and in task-list-front-end the state in App.jsx should keep track of Tasks. We want our whole app to know that a single component has been updated so we had to lift that state up to App.jsx.

You haven't lifted state up from the individual ChatEntry up into App in this project. You do use state, but we don't need state to keep track of a count of the hearts. We can calculate this value from each ChatEntry. If a value can be calculated, then we should prefer not to use state to handle this logic.

We want App.jsx to know a change has happened to ChatEntry so that React knows to re-render the each ChatEntry.

This is the state we'd like to see implemented.

  const [chatMessages, setChatMessages] = useState(messages);


const onLike = () => {
setCounter((prevCounter) => prevCounter + 1);
};

const onUnlike = () => {
setCounter((prevCounter) => prevCounter - 1);
};
Comment on lines +9 to +15
Copy link

@yangashley yangashley Dec 19, 2024

Choose a reason for hiding this comment

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

Instead of two methods for handling liking and unliking a ChatEntry, you can write an event handler that handles both scenarios.

That would look something like having an event handler that changes the state of the ChatEntrys from App and then using another helper function that looks at the state of all the ChatEntries to calculate the heart counts:

  const handleLikeData = (id) => {
    const updatedEntries = chatMessages.map(chatMessage => {
      if (chatMessage.id === id) {
        return { ...chatMessage, liked: !chatMessage.liked };
      }
      else {
        return chatMessage;
      }
    })
    setChatMessages(updatedEntries);
  };

  const calculateTotalLikes = () => 
    return chatMessages.reduce((totalLikes, chatMessage) => (totalLikes + chatMessage.liked), 0);
  };

You can see how we do this with pets on a Cat in flasky here.


const likes = counter + ' ❤️s';

return (
<div id="App">
<header>
<h2>{likes}</h2>
<h1>Application title</h1>
</header>
<main>
{/* Wave 01: Render one ChatEntry component
Wave 02: Render ChatLog component */}
{
<ChatLog
entries={messages}
onLike={onLike}
onUnlike={onUnlike} />
}
</main>
</div>
);
Expand Down
31 changes: 25 additions & 6 deletions src/components/ChatEntry.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,20 +1,39 @@
import { useState } from 'react';
import PropTypes from 'prop-types';
import './ChatEntry.css';
import TimeStamp from './TimeStamp';

const ChatEntry = ({sender, body, timeStamp, onLike, onUnlike}) => {
Copy link

@yangashley yangashley Dec 19, 2024

Choose a reason for hiding this comment

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

Suggested change
const ChatEntry = ({sender, body, timeStamp, onLike, onUnlike}) => {
const ChatEntry = ({ sender, body, timeStamp, onLike, onUnlike }) => {

Should have white spaces around curly braces when using destructuring syntax.

I'd also expect ChatLog to pass the value of liked to each ChatEntry so that this component knows if liked is true or false to set a heart color.

const [buttonText, setButtonText] = useState('🤍');
Copy link

@yangashley yangashley Dec 19, 2024

Choose a reason for hiding this comment

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

We shouldn't need to use state to keep track of the heart because we can get liked from each message. We should compute whether the heart is red or white by looking at messages liked value. We should prefer to not introduce state, which introduces additional complexity into a project, if we have some other way to accomplish the feature.


const onLikeClick = () => {
if (buttonText === '🤍') {
setButtonText('❤️');
onLike();
} else {
setButtonText('🤍');
onUnlike();
}
};
Comment on lines +9 to +17
Copy link

@yangashley yangashley Dec 19, 2024

Choose a reason for hiding this comment

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

What I'd like to see I'd like to see is an event handler written in app and this event handler would be passed down through props from App to ChatLog and finally down to ChatEntry.

ChatEntry could use this event handler passed down as a prop by registering it to the <button> element.

This is similar to how we implemented the pet-a-cat button in the Cat component in flasky-front-end. Have a look at App.jsx here from lines 44-55 to see how we write the event handler. Also have a look at Cat.jsx here from lines 17-21 to see how the passed down event handler gets used.


const ChatEntry = () => {
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} /></p>
Copy link

@yangashley yangashley Dec 19, 2024

Choose a reason for hiding this comment

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

Suggested change
<p className="entry-time"><TimeStamp time ={timeStamp} /></p>
<p className="entry-time"><TimeStamp time={timeStamp} /></p>

Nice job using the provided Timestamp component here.

Convention is to not have spaces around = sign when passing props

<button className="like" onClick={onLikeClick}>{buttonText}</button>
</section>
</div>
);
};

ChatEntry.propTypes = {
// Fill with correct proptypes
sender: PropTypes.string.isRequired,
body: PropTypes.string.isRequired,
timeStamp: PropTypes.elementType.isRequired,
onLike: PropTypes.func.isRequired,
onUnlike: PropTypes.func.isRequired,
};

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

const ChatLog = ({entries, onLike, onUnlike}) => {
Copy link

@yangashley yangashley Dec 19, 2024

Choose a reason for hiding this comment

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

Suggested change
const ChatLog = ({entries, onLike, onUnlike}) => {
const ChatLog = ({ entries, onLike, onUnlike }) => {

return (
<div>
{entries.map((entry) => (
<ChatEntry
key={entry.id}
sender={entry.sender}
body={entry.body}
timeStamp={entry.timeStamp}
onLike={onLike}
onUnlike={onUnlike}
/>
))}
</div>
);
};
Comment on lines +6 to +20
Copy link

@yangashley yangashley Dec 19, 2024

Choose a reason for hiding this comment

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

It's more typical to see the result of using map referenced by a variable and using that variable in the JSX, like:

    const chatEntries = entries.map((entry) => {
	      return (
	          <ChatEntry                                 
	            key={entry.id}
	            id={entry.id}
	            sender={entry.sender}
	            body={entry.body}
	            timeStamp={entry.timeStamp}
	            liked={entry.liked}
	            handleLike={handleLike}
	            currentUser={currentUser}
	          />
	        );
	     });
    };

  return (
        <div>
           {chatEntries}
        </div>
  );


ChatLog.propTypes = {
entries: PropTypes.arrayOf(
PropTypes.shape({
sender: PropTypes.string.isRequired,
body: PropTypes.string.isRequired,
timeStamp: PropTypes.node.isRequired,
}).isRequired
).isRequired,
onLike: PropTypes.func.isRequired,
onUnlike: PropTypes.func.isRequired,
};

export default ChatLog;