Skip to content

completed tasks#4271

Open
qlaudy wants to merge 3 commits intomate-academy:masterfrom
qlaudy:develop
Open

completed tasks#4271
qlaudy wants to merge 3 commits intomate-academy:masterfrom
qlaudy:develop

Conversation

@qlaudy
Copy link

@qlaudy qlaudy commented Feb 12, 2026

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

I am requesting changes because there are still critical issues that will break tests and violate core task requirements. The main blockers are:

  • Exports/Imports: Welcome.jsx, Header.jsx, and Article.jsx all use named exports, but the task explicitly requires default exports for all components, and App.jsx imports them with named imports. This will cause import errors in tests. Fix this by making each component a default export (e.g. export default function Header() { ... }) and updating App.jsx to use default imports like import Header from './components/Header/Header';.
  • Navigation component requirement: The <nav> markup and .navigation__link styles are still inside Header.jsx and Header.css, but the requirements say the navigation must be moved into its own Navigation component with its own folder and CSS. Create src/components/Navigation/Navigation.jsx and Navigation.css, move the <nav> block and related CSS there, export Navigation as default, and import it into Header.
  • CSS splitting: Article.css still contains .header styles inside a media query, which breaks the “CSS per component” rule. Move all .header rules to Header.css so Article.css contains only article-related selectors.
  • JSX syntax issues: In src/App.jsx, there are stray commas between components (e.g. after <Welcome /> and <Article />), which will cause invalid JSX or unwanted output. Remove those commas so your JSX children are just listed one after another.

Once you’ve made these changes, your structure will align much better with the requirements, and tests should be able to run against your components correctly. You’ve already done good work setting up component folders and per-component CSS; addressing these points will complete the task cleanly.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Important

Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.

Tip

If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.

Caution

Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment on lines 21 to 26
@media (min-width: 500px) {
.header {
text-align: left;
display: flex;
justify-content: space-between;
}

Choose a reason for hiding this comment

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

This file includes .header rules inside the media query. This violates the requirement: "CSS code should be split too." Move the .header styles into the Header component's CSS (for example src/components/Header/Header.css) so each component's styles are kept in its own folder/file.

Comment on lines 23 to 36
.navigation__link {
display: inline-block;
outline: none;
text-decoration: none;
opacity: 0.7;
padding: 0 0.5em;
color: black;
transition: opacity 0.2s ease-in-out;
}

.navigation__link:hover,
.navigation__link:focus {
opacity: 1;
}

Choose a reason for hiding this comment

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

This file contains navigation styles (.navigation__link) which violates the core requirements: "Move the navigation into its own Navigation component (you may create a folder for the Navigation component and move navigation there)." and "CSS code should be split too." Move these rules to a Navigation component folder (e.g., src/components/Navigation/Navigation.css) and import them there.


import './Article.css';

export const Article = () => (

Choose a reason for hiding this comment

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

This file uses a named export (export const Article) which violates the task requirement: "Tests expect you to use export default for all the components." Change to a default export so tests can import the component as expected.

at interdum enim lacus eu diam.
</p>
</article>
);

Choose a reason for hiding this comment

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

Add a default export for the component (for example, add export default Article; after the component definition) or change the declaration to export default function Article() { ... }. This ensures the component meets the Technical Constraint: "Tests expect you to use export default for all the components."

src/App.jsx Outdated
@@ -1,86 +1,17 @@
import React from 'react';
import './App.css';
import { Welcome } from './components/Welcome/Welcome';

Choose a reason for hiding this comment

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

You're using a named import for Welcome: import { Welcome } from './components/Welcome/Welcome';. The task's Technical Constraints say: Tests expect you to use export default for all the components. Export Welcome as default and import it as a default import: import Welcome from './components/Welcome/Welcome';

src/App.jsx Outdated
diam.
</p>
</article>
<Welcome />,

Choose a reason for hiding this comment

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

Remove the comma after <Welcome />. Having a raw comma between JSX elements is incorrect here and will either render a stray comma or cause syntax/formatting problems. Render components without commas: <Welcome />

src/App.jsx Outdated
</article>
<Welcome />,
<Header />
<Article />,

Choose a reason for hiding this comment

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

Remove the trailing comma after <Article />. The commas around JSX children are incorrect — keep the child elements adjacent without commas.


import './Welcome.css';

export const Welcome = () => (

Choose a reason for hiding this comment

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

This violates the Technical Constraints which state: Tests expect you to use export default for all the components. You currently export Welcome as a named export. Convert this to a default export (for example, define const Welcome = () => (...) and then export default Welcome; or use export default function Welcome() { ... }).


import './Header.css';

export const Header = () => (

Choose a reason for hiding this comment

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

This violates the Technical Constraints: "Tests expect you to use export default for all the components." You currently use a named export export const Header = .... Change to a default export (for example: const Header = () => (...) then export default Header) so tests can import the component as expected.

export const Header = () => (
<header className="header">
<h1 className="header__title">Site Name</h1>
<nav className="navigation">

Choose a reason for hiding this comment

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

This violates the Core Functional Requirements: "Move the navigation into its own Navigation component (you may create a folder for the Navigation component and move navigation there)." The <nav> block is implemented inside Header — extract it into components/Navigation/Navigation.jsx (with its own CSS) and import that new default-exported Navigation component into Header.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

Your solution is approved because all core requirements of the task are implemented correctly and there are no critical blockers. You’ve split the UI into separate components (Welcome, Header, Article, Navigation), each in its own folder with a default export, and App.jsx imports and renders them as required. CSS is properly split per component (Welcome.css, Header.css, Article.css, Navigation.css), with navigation styles moved out of the header file, so the “CSS per component” checklist item is satisfied. The Header correctly imports and uses the Navigation component, and each component returns valid JSX without obvious runtime issues.

The main next step is to run the test suite with npm test and confirm everything passes, since that’s part of the usual workflow even though there’s no current evidence of failures. As a minor improvement, you can clean up any leftover placeholder comments and ensure Prettier/format-on-save is enabled so future changes stay consistently formatted. Overall, this is a clear, well-structured implementation that aligns with the task’s checklist and demonstrates good component organization.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

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