Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one core requirement of the task is not yet met: the navigation has not been moved into its own Navigation component with its own CSS.
Critical issues to fix (blockers):
- In
src/components/Header/Header.jsx, the<nav className="navigation">...</nav>block is still defined insideHeader. The task explicitly requires moving the navigation into a separateNavigationcomponent. Createsrc/components/Navigation/Navigation.jsx, move the<nav>markup there, export it withexport default Navigation, and then import and render<Navigation />insideHeaderinstead of the inline<nav>. - In
src/components/Header/Header.css, there are navigation-specific styles such as.navigation__linkand its hover/focus rules. The task also requires splitting CSS per component, so you should move all navigation-related rules into a newsrc/components/Navigation/Navigation.cssand remove them fromHeader.css. Then importNavigation.cssinsideNavigation.jsx(not inHeader.jsx).
Apart from this structural issue, your decomposition of App, Article, and Welcome looks good: each has its own folder, JSX, and CSS file with export default used correctly. Once you extract the navigation component and its styles as described, your solution should align well with the task requirements. Keep going — you’re very close, and this refactor is a valuable practice in clean component structure.
✨ 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
src/components/Header/Header.css
Outdated
| .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; |
There was a problem hiding this comment.
This file contains navigation-specific styles (.navigation__link and its hover/focus rule). This violates the description requirement: "Split the App.js into components based on CSS blocks. CSS code should be split too." and specifically: "Move the navigation into a separate Navigation component (you may also create a folder for the Navigation component and move navigation there)." Consider moving these rules into Navigation.css inside a new Navigation folder and removing them from Header.css. Also update Header.jsx to import and use the new Navigation component (exported with export default).
src/components/Header/Header.jsx
Outdated
| <nav className="navigation"> | ||
| <a className="navigation__link" href="#about"> | ||
| About | ||
| </a> | ||
| <a className="navigation__link" href="#services"> | ||
| Services | ||
| </a> | ||
| <a className="navigation__link" href="#contact"> | ||
| Contact | ||
| </a> | ||
| </nav> |
There was a problem hiding this comment.
The <nav> block is defined inside Header. This violates the task description requirement: "Move the navigation into a separate Navigation component (you may also create a folder for the Navigation component and move navigation there)." Extract the <nav className="navigation">...</nav> into a new Navigation component (in its own folder) and import that component into Header instead of keeping the markup inline.
| @@ -1,5 +1,25 @@ | |||
| // import a css file containig header styles | |||
| import './Header.css'; | |||
There was a problem hiding this comment.
The file currently imports ./Header.css. Since navigation styles should be separated, this violates checklist item #2: "Create separate folder per component, where you could put all files(styles, components, and so on)". Move navigation-related CSS rules into Navigation.css inside the new Navigation folder and remove them from Header.css; then import the new CSS from the Navigation component.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because it cleanly decomposes the app into the required components (Header, Navigation, Welcome, Article) and gives each its own JSX and CSS file with appropriate imports and export default usage. The Header component correctly delegates navigation to a separate Navigation component, and navigation-related styles have been moved out of Header.css into Navigation.css, matching the requirement to split both markup and styles per component. App.jsx now composes these components in a clear, top-level structure without any obvious syntax or runtime issues.
Nothing in the current codebase appears to block task completion or break functionality based on the files reviewed. The only remaining points are project-level best practices: make sure you run npm test locally and resolve any failures, and use Prettier/VS Code settings as required so formatting stays consistent across files. Overall, this is a solid, well-structured implementation of the decomposition task.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
DEMO LINK