Skip to content

Possum: Jesica G#31

Open
jguall421 wants to merge 2 commits intoAda-C24:mainfrom
jguall421:main
Open

Possum: Jesica G#31
jguall421 wants to merge 2 commits intoAda-C24:mainfrom
jguall421:main

Conversation

@jguall421
Copy link
Copy Markdown

No description provided.

import { DateTime } from 'luxon';

const ChatEntry = () => {
const ChatEntry = ({id,sender,body,timeStamp,onLikes, liked}) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👀 Nitpick: the spacing is inconsistent here because you're missing whitespaces in several places.

Suggested change
const ChatEntry = ({id,sender,body,timeStamp,onLikes, liked}) => {
const ChatEntry = ({ id, sender, body, timeStamp, onLikes, liked }) => {

<button className="like">🤍</button>
</section>
</replace-with-relevant-semantic-element>
<section className="chat-entry local">
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It was an optional enhancement, but how could you determine if a chat was from a 'local' sender vs a 'remote' sender so that the chat bubbles would have different styles applied to it?

If you have time to re-visit this project for React.js review, it would be good practice to try applying local vs. remote classes to the components so they render differently depending on who sent the message.

<h2 className="entry-name">{sender}</h2>
<div className="entry-bubble">
<p className="entry-body">{body}</p>
<p className="entry-time"><TimeStamp time={timeStamp}/></p>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nice work using the provided TimeStamp component 👍

import PropTypes from 'prop-types';

const ChatLog =({entries, onLikeEntry }) => {
const ChatEntryLogs = entries.map(entry=> {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ChatEntryLogs isn't a component but a variable, so we can use standard camelCase to name it to avoid any confusion.

Suggested change
const ChatEntryLogs = entries.map(entry=> {
const chatEntryLogs = entries.map(entry=> {

timeStamp={entry.timeStamp}
liked={entry.liked}
onLikes={onLikeEntry}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nitpick: Remove unnecessary blank line

Suggested change

Comment thread src/App.jsx
const handleLikes = id => {
setMessages(prevMessages=>{
return prevMessages.map(entry => {
if(entry.id === id){
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You'll want to fix up code style issues while you're writing code and use a linter to check for style issues before submitting code for review.

Suggested change
if(entry.id === id){
if (entry.id === id) {

Comment thread src/App.jsx
return (
<div id="App">
<header>
<h1>Chat between Vladimir and Estragon</h1>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you think of a way to dynamically get the senders' names without hardcoding "Vladimir" and "Estragon" here?

Comment thread src/App.jsx
<div id="App">
<header>
<h1>Chat between Vladimir and Estragon</h1>
<h2>{`${totalLikes} ❤️s`}</h2>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can directly invoke countTotalLikes here instead of creating a variable first since the logic is straight forward and doesn't compromise the readability.

Suggested change
<h2>{`${totalLikes} ❤️s`}</h2>
<h2>{`${countTotalLikes(messages)} ❤️s`}</h2>

import ChatEntry from './ChatEntry';
import PropTypes from 'prop-types';

const ChatLog =({entries, onLikeEntry }) => {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👀 Nitpick: issue with whitespacing

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

Comment thread src/App.jsx
};

function App() {
const[messages, setMessages] = useState(messagesData);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
const[messages, setMessages] = useState(messagesData);
const [messages, setMessages] = useState(messagesData);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants