C24 Possum - React-chatlog - YanYi Chen by yan202509 · Pull Request #19 · Ada-C24/react-chatlog

yan202509 · 2025-12-16T03:22:38Z

No description provided.

…lay a sequence of ChatEntry components

…hatEntry to App

anselrognlie

Your implementation looks good overall! Please review my comments, and let me know if you have any questions.

anselrognlie · 2026-01-12T18:41:04Z

    <div id="App">
      <header>
-        <h1>Application title</h1>
+        <h1>Chat between Two Person</h1>


It would be nice to update this to at least a hard-coded representation of the conversation title:

<h1>Chat between Vladimir and Estragon</h1>

anselrognlie · 2026-01-13T00:32:42Z

+  id: PropTypes.number.isRequired,
+  sender: PropTypes.string.isRequired,
+  body: PropTypes.string.isRequired,
+  timeStamp: PropTypes.string.isRequired,
+  liked: PropTypes.bool.isRequired,


The id, sender, body, timeStamp, and liked props are always passed (they're defined explicitly in the data and also provided in the test) so we can (and should) mark them isRequired.

The remaining props were up to you, and the tests don't know about them. As a result, using isRequired would cause a warning when running any tests that only pass the known props. As a result, either leaving the prop not marked isRequired or updating the tests to pass a dummy function is required to avoid the warnings.

If we do leave props marked not isRequired, we should either have logic to avoid using those values if they are missing, or make allowance for using a default value.

anselrognlie · 2026-01-13T00:33:05Z

+import PropTypes from 'prop-types';
+import TimeStamp from './TimeStamp';
+
+const ChatEntry = (props) => {


Personally, I prefer to destructure the props since that helps give a clear picture of the props that are actually used within the component in one place which is actually tied to the implementation (as opposed to the PropTypes, which is more loosely connected, and deprecated in more recent React versions). When accessing values on the props instance directly, we need to look throughout the body of the component to see what fields on props are actually used.

anselrognlie · 2026-01-13T00:33:28Z

+const ChatEntry = (props) => {
+
+  const likeButtonClicked = () => {
+    props.onEachLikeToggle(props.id);


Note that for any props that aren't required (e.g., onEachLikeToggle), we should check that a value was supplied before trying to use it. For example, trying to call a function reference if the value is undefined will result in an error.

anselrognlie · 2026-01-13T00:33:48Z

-        <button className="like">🤍</button>
+        <p>{props.body}</p>
+        <p className="entry-time">
+          <TimeStamp time={props.timeStamp}></TimeStamp>


Note that for components that don't expect to be given child markup, rather than explicitly writing the closing tag <TimeStamp></TimeStamp>, we prefer to use the empty tag shorthand <TimeStamp />.

anselrognlie · 2026-01-13T00:35:53Z

    // Replace the outer tag name with a semantic element that fits our use case
-    <replace-with-relevant-semantic-element className="chat-entry local">
-      <h2 className="entry-name">Replace with name of sender</h2>
+    <main className="chat-entry local">


👀 Note that main can be semantic when used in an appropriate context, but here, it causes a validation issue. We need to keep in mind that the React components that we create eventually become HTML displayed in a browser, and we should ensure that the resulting HTML is valid. There should be only a single main tag on a page, but there will me several ChatEntry components rendered, each of which will have its own main tag.

Rather than using main here, there are two potential semantic tags that would arguably be more appropriate (each which usually recommends including a heading tag h of some level). Prefer to use one of those.

anselrognlie · 2026-01-14T00:28:45Z

+  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,
+    }),
+  ),
+  onLikeToggle: PropTypes.func,


Similar to the props for ChatEntry, here, we have some props that the tests pass and some they don't. The ones that the tests pass (entries) can be marked required without issue, while marking anything else required would cause warnings. Alternatively, we could update the tests to pass these additional props once we have defined them.

We should remember that if we leave props not required, we should include appropriate logic to check for a value before making use of it (e.g., we shouldn't try to call an optional callback without ensuring we have a function to call!).

anselrognlie · 2026-01-14T00:35:56Z

+  });
+
+  return (
+    <ul>


Nice use of ul with li to encode the structure of the conversation as a list of chat entries. Since ChatEntry is itself better represented by a non-li semantic tag, if we want to use a list to build the onversation, then wrapping the li around theChatEntry works well.

Note that sine we don't want the bullets from the default list styling to show in our conversation, we would need to add additional style rules to remove them.

anselrognlie · 2026-01-14T00:35:59Z

+const ChatLog = (props) => {
+  const chatEntryComponents = props.entries.map((message) => {
+    return (
+      <li key={message.id}>


👍 Good job recognizing that the key value then goes on the li item rather than our ChatEntry. Any tag that's rendered into an array, whether a custom component of our own, or a basic component provided by React, should be given a key value.

anselrognlie · 2026-01-14T00:36:19Z

+import PropTypes from 'prop-types';
+
+const ChatLog = (props) => {
+  const chatEntryComponents = props.entries.map((message) => {


Nice use of map to convert from the message data into ChatEntry components. We can perform this mapping storing the result into a variable we use in the JSX result as you did here (components are functions, so we can run JS code as usual before we reach the return, and even sometimes have multiple return statements with different JSX), we could make a helper function that we call as part of the return, or this expression itself could be part of the return JSX, which I often like since it helps me see the overall structure of the component, though it can make debugging a little more tricky. But any of those approaches will work fine.

…for names, change main to article, so main is only with app.jsx

anselrognlie

👍 Thanks for the update. Looks good.

anselrognlie · 2026-01-15T06:16:55Z

  return (
    // Replace the outer tag name with a semantic element that fits our use case
-    <main className="chat-entry local">
+    <article className="chat-entry local">


👍 article is a great choice for a semantic tag, since one of its uses is to represent self-contained, reusable page elements.

anselrognlie · 2026-01-15T06:17:29Z

  background-color: red;
 }
+
+li {


👍 This helps the styling significantly.

anselrognlie · 2026-01-15T06:18:26Z

+  const user1 = 'Vladimir';
+  const user2 = 'Estragon';


Rather than hard coding these names, could we calculate them from the message data? More generally, think about what we might do if there were more than two participants in the conversation (a group chat).

yan202509 added 4 commits December 13, 2025 17:52

wave 1 complete, use presentational component ChatEntry with App

9bbcc88

wave 2 complete, ChatLog has one prop named entries (array), and disp…

50570aa

…lay a sequence of ChatEntry components

wave 3 is complete, with the use of props and lifting up state from C…

32792f6

…hatEntry to App

the toggle function in ChatEntry and ChatLog is not set to required

99c6a9b

anselrognlie reviewed Jan 14, 2026

View reviewed changes

change 'chat user name' on top, remove list style to no bullet point …

945b1aa

…for names, change main to article, so main is only with app.jsx

anselrognlie reviewed Jan 15, 2026

View reviewed changes

Conversation

yan202509 commented Dec 16, 2025

Uh oh!

anselrognlie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anselrognlie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants