Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
64 changes: 0 additions & 64 deletions src/App.css
Original file line number Diff line number Diff line change
Expand Up @@ -2,73 +2,9 @@
background-color: #87cefa;
}

#App header {
background-color: #222;
color: #fff;
padding-bottom: 0.5rem;
position: fixed;
width: 100%;
z-index: 100;
text-align: center;
align-items: center;
}

#App main {
padding-left: 2em;
padding-right: 2em;
padding-bottom: 5rem;
padding-top: 10rem;
}

#App h1 {
font-size: 1.5em;
text-align: center;
display: inline-block;
}

#App header section {
background-color: #e0ffff;
}

#App .widget {
display: inline-block;
line-height: 0.5em;
border-radius: 10px;
color: black;
font-size:0.8em;
padding-left: 1em;
padding-right: 1em;
}

#App #heartWidget {
font-size: 1.5em;
margin: 1em
}

#App span {
display: inline-block
}

.red {
color: #b22222
}

.orange {
color: #e6ac00
}

.yellow {
color: #e6e600
}

.green {
color: green
}

.blue {
color: blue
}

.purple {
color: purple
}
49 changes: 44 additions & 5 deletions src/App.jsx
Original file line number Diff line number Diff line change
@@ -1,14 +1,53 @@
import './App.css';
import { useState } from 'react';
import ChatLog from './components/ChatLog.jsx';
import messages from './data/messages.json';
import Header from './components/Header.jsx';

const App = () => {
const [entries, setEntries] = useState(messages);
const [fontColorForLocalSender, setFontColorForLocalSender] = useState('black');
const [fontColorForRemoteSender, setFontColorForRemoteSender] = useState('black');
Comment on lines +9 to +10
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

❗️👀 The same concept (“what color does each sender use?”) is being represented in 3 components (App, Header, and SenderColorPicker), which is a strong signal that state placement needs to be cleaned up. There is no longer a single source of truth, but 3 sources of truth!

Again, my eyebrows were raised when I saw another place where there is state that is related to color.

Now that I see how the 3 components work together, I see that SenderColorPicker only needs to trigger a change. It does not need to “own” or persist the color itself and Header doesn't need to either.

When I think about App:

  • It should own the source of truth for sender colors (this is where the state for colors should live)
  • It should decides how many senders exist and what their colors are
  • It passes the current colors down
  • It should have a function for updating colors and passes it down

Header should:
- Have no color state (remove it)

  • Be purely presentational
  • Receives colors as props
  • Passes the event handler callback related to colors down to the picker

SenderColorPicker should:
- Have no color state (remove it)

  • Have no business logic because it only displays the UI
  • Notify when a color is chosen

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧠 We should identify all the components need to know about color and identify their closest common ancestor.

We should also really consider what each component should know and it should be responsible for. This is why we put so much emphasis on pre-planning your components during the activity related to task-list-front-end (where the first wave was looking at a mockup and identifying what components you needed and how data should flow between the components).

For future projects it will be critical to do this kind of planning for your components before you start writing code so you don't create many different sources of truth by introducing redundant pieces of state.

const localSender = 'Vladimir';

const totalLikes = entries.reduce((total, entry) => {
return entry.isLiked ? total + 1 : total;
}, 0);

const toggleHeart = (entryId) => {
setEntries(entries => {
return entries.map(entry => {
Comment on lines +18 to +19
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 job using the callback-style for updating the state.

if (entry.id === entryId) {
return { ...entry, isLiked: !entry.isLiked };
} else {
return entry;
}
});
});
};

const chooseFontColor = (senderName, color) => {
if (senderName === localSender) {
setFontColorForLocalSender(color);
} else {
setFontColorForRemoteSender(color);
}
};

return (
<div id="App">
<header>
<h1>Application title</h1>
</header>
<Header
totalLikes={totalLikes}
chooseFontColor={chooseFontColor}
/>
<main>
{/* Wave 01: Render one ChatEntry component
Wave 02: Render ChatLog component */}
<ChatLog
entries={entries}
localSender={localSender}
onToggleHeart={toggleHeart}
fontColorForLocalSender={fontColorForLocalSender}
fontColorForRemoteSender={fontColorForRemoteSender}
/>
</main>
</div>
);
Expand Down
16 changes: 15 additions & 1 deletion src/components/ChatEntry.css
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ button {
background: none;
color: inherit;
border: none;
padding: 10px;
padding: 10px;
font: inherit;
cursor: pointer;
outline: inherit;
Expand Down Expand Up @@ -97,4 +97,18 @@ button {

.chat-entry.remote .entry-bubble:hover::before {
background-color: #a9f6f6;
}

.entry-attributes {
display: flex;
align-items: center;
justify-content: space-between;
}

.like {
padding: 0;
width: 20px;
height: 20px;
align-items: center;
line-height: 1;
}
43 changes: 31 additions & 12 deletions src/components/ChatEntry.jsx
Original file line number Diff line number Diff line change
@@ -1,21 +1,40 @@
import './ChatEntry.css';
import PropTypes from 'prop-types';
import TimeStamp from './TimeStamp';

const ChatEntry = (props) => {
const isLocal = props.sender === props.localSender;
Comment on lines +5 to +6
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Destructuring props can be a matter of preference. I prefer to destructure props so that I don't need to type props. every time I want to get the attribute from the object.

Destructuring props also has some benefits:

  • you know every possible prop right away.
  • you know when a prop is being unused, in order to remove it.
  • knowing which props are being used will also remind you what you need to validate (I see a review comment from copilot for line 39 below that calls out props that weren't validated with propTypes).

Ultimately, you'll adapt what you do depending on what your team does.

Suggested change
const ChatEntry = (props) => {
const isLocal = props.sender === props.localSender;
const ChatEntry = ( {sender, localSender} ) => {
const isLocal = sender === localSender;

const entryClass = isLocal ? 'local' : 'remote';
const isLiked = props.isLiked ? '❤️' : '🤍';
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

The variable name 'isLiked' is misleading as it stores the emoji string to display, not a boolean value. Consider renaming it to 'likeIcon' or 'heartIcon' to better reflect what it contains.

Copilot uses AI. Check for mistakes.
const isLikedButtonClicked = () => {
props.onToggleHeart(props.id);
};
Comment on lines +9 to +11
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

The function name 'isLikedButtonClicked' uses a naming pattern that describes state rather than an action. Consider renaming to 'handleLikeClick' or 'handleToggleLike' to better indicate the action being performed and follow React naming conventions for event handlers.

Copilot uses AI. Check for mistakes.

const fontColor = isLocal ? props.fontColorForLocalSender : props.fontColorForRemoteSender;

const ChatEntry = () => {
return (
// 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>
<article className={`chat-entry ${entryClass}`}>
<h2 className="entry-name">{props.sender}</h2>
<section className="entry-bubble">
Comment on lines +17 to 18
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

While not strictly required by the HTML specification, it is a strong best practice and highly recommended that section elements should have a heading element inside them.

You could put this h2 on line 17 inside the section on line 18 to fulfill this specification.

<p>Replace with body of ChatEntry</p>
<p className="entry-time">Replace with TimeStamp component</p>
<button className="like">🤍</button>
<p className={`entry-body ${fontColor}`}>{props.body}</p>
<div className="entry-attributes">
<p className="entry-time">
<TimeStamp time={props.timeStamp} />
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 job using the supplied TimeStamp component here 👍

</p>
<button className="like" onClick={isLikedButtonClicked}>{isLiked}</button>
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

The like button only uses emoji without an accessible label. Screen reader users won't understand the button's purpose. Add an aria-label attribute (e.g., aria-label="Like message" or aria-label="Unlike message" based on the current state) to improve accessibility.

Copilot uses AI. Check for mistakes.
</div>
</section>
</replace-with-relevant-semantic-element>
</article>
);
};

ChatEntry.propTypes = {
// Fill with correct proptypes
};

export default ChatEntry;

ChatEntry.propTypes = {
sender: PropTypes.string.isRequired,
body: PropTypes.string.isRequired,
timeStamp: PropTypes.string.isRequired,
localSender: PropTypes.string,
isLiked: PropTypes.bool,
onToggleHeart: PropTypes.func,
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

The PropTypes definition is incomplete and doesn't match the component's actual props. The component receives 'id', 'fontColorForLocalSender', and 'fontColorForRemoteSender' props (lines 10, 13) but these are not defined in PropTypes. Add these missing prop type definitions to ensure proper type checking.

Suggested change
onToggleHeart: PropTypes.func,
onToggleHeart: PropTypes.func,
id: PropTypes.oneOfType([PropTypes.string, PropTypes.number]).isRequired,
fontColorForLocalSender: PropTypes.string.isRequired,
fontColorForRemoteSender: PropTypes.string.isRequired,

Copilot uses AI. Check for mistakes.
};
39 changes: 39 additions & 0 deletions src/components/ChatLog.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import './ChatLog.css';
import ChatEntry from './ChatEntry';
import PropTypes from 'prop-types';

const ChatLog = ({entries, localSender, onToggleHeart, fontColorForLocalSender, fontColorForRemoteSender}) => {
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 destructuring your props here 👍

return (
<section className="chat-log">
{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.

Another common way you'll see components organized using map is to store the result into a variable we use in the JSX result:

  const chatEntries = entries.map((entry) => {
    return (
      <ChatEntry
        // your props
      />
    );
  });

  return <section className="chat-log">{chatEntries}</section>;

<ChatEntry
key={entry.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.

👍 The key attribute is important for React to be able to detect certain kinds of data changes in an efficient manner. We're also using the id for our own id prop, so it might feel redundant to pass both, but one is for our logic and one is for React internals.

id={entry.id}
sender={entry.sender}
body={entry.body}
timeStamp={entry.timeStamp}
localSender={localSender}
isLiked={entry.isLiked}
onToggleHeart={onToggleHeart}
fontColorForLocalSender={fontColorForLocalSender}
fontColorForRemoteSender={fontColorForRemoteSender}
/>
))}
</section>
);
};

export default ChatLog;

ChatLog.propTypes = {
entries: PropTypes.arrayOf(
PropTypes.shape({
id: PropTypes.number.isRequired,
sender: PropTypes.string.isRequired,
body: PropTypes.string.isRequired,
timeStamp: PropTypes.string.isRequired,
})
).isRequired,
localSender: PropTypes.string,
onToggleHeart: PropTypes.func,
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

The PropTypes definition is incomplete and doesn't match the component's actual props. The component receives 'fontColorForLocalSender' and 'fontColorForRemoteSender' props (lines 5, 18-19) but these are not defined in PropTypes. Add these missing prop type definitions to ensure proper type checking.

Suggested change
onToggleHeart: PropTypes.func,
onToggleHeart: PropTypes.func,
fontColorForLocalSender: PropTypes.string,
fontColorForRemoteSender: PropTypes.string,

Copilot uses AI. Check for mistakes.
};
20 changes: 20 additions & 0 deletions src/components/Header.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#App header {
background-color: #2c2c2c;
color: #fff;
padding-bottom: 0.5rem;
position: fixed;
width: 100%;
z-index: 100;
text-align: center;
align-items: center;
}

#App h1 {
font-size: 1.5em;
text-align: center;
display: inline-block;
}

#App header section {
background-color: #e0ffff;
}
34 changes: 34 additions & 0 deletions src/components/Header.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { useState } from 'react';
import './Header.css';
import HeartCounter from './HeartCounter.jsx';
import SenderColorPicker from './SenderColorPicker.jsx';

const Header = ({ totalLikes, chooseFontColor }) => {
const [localColor, setLocalColor] = useState('white');
const [remoteColor, setRemoteColor] = useState('white');
Comment on lines +7 to +8
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Instead of using 2 pieces of state to track colors for the local and remote sender, we can use one piece with an object for the initial state.

Wrt to state, it's beneficial to evaluate what pieces of state do we really need and how can we simplify state so that our project won't be overly complicated.

Suggested change
const [localColor, setLocalColor] = useState('white');
const [remoteColor, setRemoteColor] = useState('white');
const [senderColor, setSenderColor] = useState(
{
local: 'white',
remote: 'white'
}
);


const changeFontColor = (senderName, color) => {
if (senderName === 'Vladimir') {
setLocalColor(color);
} else {
setRemoteColor(color);
}
chooseFontColor(senderName, color);
};

return (
<header>
<h1>
Chat between <span className={localColor}>Vladimir</span> and <span className={remoteColor}>Estragon</span>
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?

</h1>

<section>
<SenderColorPicker senderName="Vladimir" chooseFontColor={changeFontColor} />
<HeartCounter totalLikes={totalLikes} />
<SenderColorPicker senderName="Estragon" chooseFontColor={changeFontColor} />
</section>
</header>
);
};

export default Header;
18 changes: 18 additions & 0 deletions src/components/HeartCounter.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#App .widget {
display: inline-block;
line-height: 0.5em;
border-radius: 10px;
color: black;
font-size: 0.8em;
padding-left: 1em;
padding-right: 1em;
}

#App #heartWidget {
font-size: 1.5em;
margin: 1em;
}

#App span {
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

The CSS selector '#App span' is too broad and will affect all span elements throughout the entire application. This could cause unintended styling side effects on other components. Consider using a more specific selector scoped to this component.

Suggested change
#App span {
#App #heartWidget span {

Copilot uses AI. Check for mistakes.
display: inline-block;
}
Comment on lines +1 to +18
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

The CSS rules defined here (.widget, #heartWidget, span) appear to be component-specific styles but are placed in HeartCounter.css. Since these styles are shared across multiple components (HeartCounter and potentially SenderColorPicker use .widget class), consider moving these to a shared stylesheet or the main App.css file to avoid duplication and maintain consistency.

Copilot uses AI. Check for mistakes.
11 changes: 11 additions & 0 deletions src/components/HeartCounter.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import './HeartCounter.css';

const HeartCounter = ({ totalLikes }) => {
return (
<div className="widget" id="heartWidget">
{totalLikes} ❤️s
</div>
);
};

export default HeartCounter;
35 changes: 35 additions & 0 deletions src/components/SenderColorPicker.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
span {
display: inline-block
}
Comment on lines +1 to +3
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

The CSS selector 'span' is too broad and will affect all span elements globally in the application. This should be scoped to avoid unintended side effects on other components. Consider using a more specific selector like '.sender-color-picker span' to limit the scope to this component only.

Copilot uses AI. Check for mistakes.

.red {
color: #b22222
}

.orange {
color: #e6ac00
}

.yellow {
color: #e6e600
}

.green {
color: green
}

.blue {
color: blue
}

.purple {
color: purple
}

.black {
Comment on lines +5 to +29
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

The color utility classes (red, orange, yellow, green, blue, purple, black) are defined globally without scoping. These classes will affect any element with these class names across the entire application. Consider either scoping them to this component (e.g., '.sender-color-picker .red') or moving them to a shared utility CSS file if they're intended to be used globally.

Suggested change
.red {
color: #b22222
}
.orange {
color: #e6ac00
}
.yellow {
color: #e6e600
}
.green {
color: green
}
.blue {
color: blue
}
.purple {
color: purple
}
.black {
.senderColorPicker .red {
color: #b22222
}
.senderColorPicker .orange {
color: #e6ac00
}
.senderColorPicker .yellow {
color: #e6e600
}
.senderColorPicker .green {
color: green
}
.senderColorPicker .blue {
color: blue
}
.senderColorPicker .purple {
color: purple
}
.senderColorPicker .black {

Copilot uses AI. Check for mistakes.
color: black
}

.senderColorPicker span {
Copy link

Copilot AI Dec 16, 2025

Choose a reason for hiding this comment

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

The selector '.senderColorPicker' uses camelCase but doesn't match the actual class name 'sender-color-picker' (kebab-case) used in the JSX component. This CSS rule will not apply to any elements. The selector should be '.sender-color-picker span' to match the component's class name.

Suggested change
.senderColorPicker span {
.sender-color-picker span {

Copilot uses AI. Check for mistakes.
margin: 0 0.15em;
}
Loading