Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Decision: REQUEST_CHANGES
The submission is close, but a core requirement is not met: the navigation must be extracted into a separate Navigation component with its own styles, and used in the app. This is a critical blocker for checklist compliance and may affect tests.
Critical issues to fix
-
Navigation not extracted into its own component (violates requirement 1.3 and checklist item 2)
- Locations:
src/components/Header/Header.jsx: Navigation markup is still inline. Likely around the<nav>...</nav>block.src/App.jsx: No import/usage ofNavigation.
- Why it’s an issue: The task explicitly requires moving navigation into
Navigationfor modularity and testability. - How to fix:
- Create
src/components/Navigation/Navigation.jsxwith a default exportNavigationthat renders the previous nav markup. - Create
src/components/Navigation/Navigation.cssand import it inNavigation.jsx. - In
Header.jsx, remove the inline nav markup and import/useNavigationinstead:import Navigation from "../Navigation/Navigation";then render<Navigation />where the<nav>was. - Alternatively, if the design expects navigation at the App level, import and render
<Navigation />insrc/App.jsxin the appropriate spot.
- Create
- Locations:
-
Navigation CSS mixed into Header CSS (violates requirement 1.4 and checklist item 2)
- Location:
src/components/Header/Header.css, lines 29–42 (.navigation__linkstyles). - Why it’s an issue: Each component must have its own co-located styles.
- How to fix: Move
.navigation__linkrules tosrc/components/Navigation/Navigation.cssand remove them fromHeader.css.
- Location:
-
Stray semicolon after
</nav>inHeader.jsx- Location:
src/components/Header/Header.jsx, immediately after the closing</nav>tag. - Why it’s an issue: Unnecessary syntax that could cause rendering quirks.
- How to fix: Remove the semicolon following
</nav>.
- Location:
Minor notes (nice-to-have after the above)
- After extraction, ensure all components still use default exports and imports are updated accordingly.
You’ve done a solid job decomposing Article, Header, and Welcome with clean, co-located styles. Addressing the navigation extraction and CSS placement will bring the solution fully in line with the requirements. Keep going—you’re very close!
✨ 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
| import Welcome from './components/Welcome/Welcome'; | ||
| import Article from './components/Article/Article'; | ||
| import Header from './components/Header/Header'; | ||
|
|
There was a problem hiding this comment.
This file is missing the Navigation component import and usage. The task description and checklist require that the navigation part of the UI be moved into a separate Navigation component and used here. Please import Navigation from its folder and render it in the appropriate place in the JSX.
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; | ||
| } No newline at end of file |
There was a problem hiding this comment.
This file contains styles for .navigation__link, which should be moved to the Navigation component's CSS file. Each component must have its own folder with co-located style files, and CSS should be split according to decomposed components (see checklist item 2 and requirement 1.4).
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 navigation markup should be moved to a separate Navigation component, and the Header component should import and use Navigation instead. This violates requirement 1.3 and checklist item 2.
src/components/Header/Header.jsx
Outdated
| Contact | ||
| </a> | ||
| </nav> | ||
| ; |
There was a problem hiding this comment.
There is a stray semicolon after the tag, which should be removed.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! I am approving your solution as you've successfully addressed all the previous feedback. The new Navigation component is perfectly extracted with its own co-located styles, resulting in a clean and well-structured application. The project now serves as a great example of a modular React component structure that fully meets all task requirements. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.