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

Nikki Vaughan Sphinx #45

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

Nikki Vaughan Sphinx #45

wants to merge 5 commits into from

Conversation

mikellewade
Copy link

No description provided.

Copy link
Author

@mikellewade mikellewade left a comment

Choose a reason for hiding this comment

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

Nice work! Your project lets me know that you have a goo understanding of React and useState!


const App = () => {
const [messages, setMessages] = useState(messageData);
const [likedCount, setLikedCount] = useState(0);
Copy link
Author

Choose a reason for hiding this comment

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

Do we necessarily need this piece of state? Couldn't we just use the fact that if a message is liked then we could have a function that calculates the total number of likes by iterating over the messages that is chatData? This works each render since each time a message is liked chatData state is changed.

Comment on lines +12 to +17
const toggleLikedMessage = (messageId) => {
const updatedMessages = messages.map((message) => {
return message.id === messageId
? { ...message, liked: !message.liked }
: message;
});
Copy link
Author

Choose a reason for hiding this comment

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

Nice work on this function! By passing this down to your individual chat messages you are now able to have a single source of truth!


const likedList = updatedMessages.filter((message) => message.liked);

setLikedCount(likedList.length);
Copy link
Author

Choose a reason for hiding this comment

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

Like mentioned previously, we could ditch the extra piece of state and do the following:

const totalLikes = chatMessages.reduce((count, message) => count + (message.liked ? 1 : 0), 0);

Then we could use it like so:

<p className='widget' id='heartWidget'>{totalLikes} ❤️s</p>

Comment on lines +25 to +31
const participantList = [];
messages.forEach((message) => {
if (!participantList.includes(message.sender)) {
participantList.push(message.sender);
}
});

Copy link
Author

Choose a reason for hiding this comment

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

Nice this would handle multiple (2+) participants!

Comment on lines +60 to +63
<h1>Chat between </h1>{' '}
<h1 className={localColor}>{participantList[0]}</h1>{' '}
<h1>and</h1>{' '}
<h1 className={remoteColor}>{participantList[1]}</h1>
Copy link
Author

Choose a reason for hiding this comment

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

Perhaps we could combine some of the tags here to make it more readable. Especially since we are using a lot of the same tags.

Comment on lines +78 to +83
<ChatLog
entries={messages}
onMessageLiked={toggleLikedMessage}
localColor={localColor}
remoteColor={remoteColor}
/>
Copy link
Author

Choose a reason for hiding this comment

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

⭐️

Comment on lines +27 to +37
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,
})).isRequired,
onMessageLiked: PropTypes.func.isRequired,
localColor: PropTypes.string,
remoteColor: PropTypes.string,
Copy link
Author

Choose a reason for hiding this comment

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

⭐️

Comment on lines +6 to +12
const isRemote = props.sender === 'Estragon' ? true : false;
const senderLocation = isRemote
? 'chat-entry remote'
: 'chat-entry local';
const senderColor = isRemote
? props.remoteColor
: props.localColor;
Copy link
Author

Choose a reason for hiding this comment

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

Ternary Terminal lol!

Comment on lines +23 to +25
onClick={
() => props.onMessageLiked(props.id)
}>{props.liked ? '❤️' : '🤍'}
Copy link
Author

Choose a reason for hiding this comment

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

Nice work!

Comment on lines +34 to +41
id: PropTypes.number.isRequired,
sender: PropTypes.string.isRequired,
body: PropTypes.string.isRequired,
timeStamp: PropTypes.string.isRequired,
liked: PropTypes.bool.isRequired,
onMessageLiked: PropTypes.func.isRequired,
localColor: PropTypes.string,
remoteColor: PropTypes.string,
Copy link
Author

Choose a reason for hiding this comment

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

⭐️

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