Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on decomposing the application into Header, Welcome, and Article components and separating their styles. The project structure is well-organized.
However, changes are needed because a key part of the decomposition is incomplete. The task suggests creating a separate Navigation component, but the navigation logic and styles are still inside the Header component. To complete the task, please move the <nav> element from Header.jsx and its associated styles from Header.css into a new Navigation component.
I also noticed that your components use both named and default exports. The task requires only a default export, so you can remove the export keyword from the function declarations to avoid redundancy.
Once the Navigation component is created, your solution will fully meet the requirements. Keep up the great work!
✨ 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 PageHeader from './components/Header/Header'; | ||
| import PageWelcome from './components/Welcome/Welcome'; | ||
| import PageArticle from './components/Article/Article'; |
There was a problem hiding this comment.
While this works, it's a common convention in React to name the imported component the same as the component itself. For instance, import Header from './components/Header/Header';. Using prefixes like Page can make the code slightly more verbose than necessary.
| import './Article.css'; | ||
|
|
||
| // Create an Article function returning the HTML of article block | ||
| export function Article() { |
There was a problem hiding this comment.
You're exporting this component in two ways: as a named export on this line and as a default export at the end of the file. While this works, it's a common practice to have a single export per component file. Since the task requires a default export, you can remove the export keyword from this function declaration and rely solely on the export default Article; at the bottom. Alternatively, you could combine them into one line: export default function Article() { ... }.
| .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.
Good job splitting the CSS. However, the task description suggests creating a separate Navigation component. To follow this structure, the styles related to .navigation__link should be moved to a new Navigation.css file inside the Navigation component's folder.
| import './Welcome.css'; | ||
|
|
||
| // Create a Welcome function returning the HTML of welcome block | ||
| export function Welcome() { |
There was a problem hiding this comment.
Similar to the other components, you have both a named and a default export. The task requires a default export, so the named export on this line is redundant. To simplify, you can remove the export keyword here and just use the export default Welcome; at the end of the file.
| import './Header.css'; | ||
|
|
||
| // Create a Header function returning the HTML of header block | ||
| export function Header() { |
There was a problem hiding this comment.
You have both a named export here and a default export at the end of the file. To keep the code consistent and follow the task's requirement for default exports, it's better to use only one. You can remove the export keyword from this line and just keep the export default Header; at the bottom.
| <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.
Good job creating the Header component. However, the task description suggests that the navigation part should also be a separate component. To complete the decomposition, you should create a new Navigation component in its own folder (src/components/Navigation), move this <nav> block into it, and then import and use the <Navigation /> component here.
2 participants
DEMO LINK