Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on decomposing the App into Header, Welcome, and Article components! The project structure and styling for these components are well-implemented.
However, one of the core requirements was to extract all major UI blocks into their own components, and it looks like the Navigation block was missed. To complete the task, please create a new Navigation component, move the relevant JSX and CSS into it, and then import and render it within App.jsx.
As a minor cleanup, you can also remove the leftover template comments in the component files. Keep up the great work, and I look forward to seeing the updated version!
✨ 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.jsx
Outdated
| // Create a Header function returning the HTML of header block | ||
|
|
||
| // Add a default export statement for Header component to use it in the other files |
There was a problem hiding this comment.
These comments from the task template are no longer necessary as you've already implemented the function and the default export. It's good practice to remove such instructional comments from the final code.
| <Header /> | ||
| <Article /> |
There was a problem hiding this comment.
It seems the Navigation component is missing. According to the task description, you should also create a component for the navigation block and include it here, likely between the Header and the Article.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one core task requirement is still not met: the Navigation component is not imported and rendered from App.jsx as specified. Instead, Navigation is currently used inside Header.jsx, which conflicts with the instruction to render it from App.jsx; you should move the import Navigation from './components/Navigation/Navigation' and the <Navigation /> JSX into App.jsx and remove it from Header.jsx. This is a functional/requirements issue, not just style, since the task explicitly grades on this structure.
Critical items to fix:
- App.jsx – Import
Navigationat the top with the other components and include<Navigation />in the returned JSX so the navigation block is rendered directly fromApp. Then adjustHeader.jsxso it no longer imports or rendersNavigation. - Navigation.css – Replace the placeholder comment with the actual navigation-related styles (move any nav styles from
App.css/Header.csshere) so CSS is truly split per component as required.
Minor but important cleanups (not blockers but please fix with the same commit):
- Remove the leftover scaffold/template comments at the end of
src/components/Article/Article.jsx,src/components/Header/Header.jsx, andsrc/components/Welcome/Welcome.jsxto keep the codebase clean. Overall your component structure, default exports, and JSX look good; once you adjust whereNavigationis rendered and move its styles intoNavigation.css, this will fully meet the task requirements.
✨ 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
| </p> | ||
| </article> | ||
| ); | ||
| } |
There was a problem hiding this comment.
This violates the previous-review correction: "As a minor cleanup, remove the leftover template comments in the component files." Remove the trailing scaffold/template comments at the end of this file (they're not needed and should be deleted before finalizing the PR).
| import Header from './components/Header/Header'; | ||
| import Welcome from './components/Welcome/Welcome'; | ||
| import Article from './components/Article/Article'; | ||
|
|
There was a problem hiding this comment.
This violates the task requirement: "Create a new Navigation component, move the relevant JSX and CSS into it, and then import and render it within App.jsx." Navigation is not imported in this file and not rendered in the JSX. Consider adding import Navigation from './components/Navigation/Navigation' with the other imports and including <Navigation /> in the returned JSX where appropriate so the Navigation block is rendered from App.jsx.
src/components/Header/Header.jsx
Outdated
| @@ -1,5 +1,14 @@ | |||
| // import a css file containig header styles | |||
| import './Header.css'; | |||
| import Navigation from '../Navigation/Navigation'; | |||
There was a problem hiding this comment.
This file imports and renders Navigation inside Header, but the task explicitly requires: "Create a new Navigation component, move the relevant JSX and CSS into it, and then import and render it within App.jsx." If the requirement expects Navigation to be rendered from App.jsx, move the import Navigation and <Navigation /> usage to App.jsx (or clarify the requirement if Header should host Navigation).
| <Navigation /> | ||
| </header> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Remove the leftover template/scaffolding comments at the end of the file (they are present after the component). Clean up these lines before submitting the PR.
| @@ -0,0 +1 @@ | |||
| /* Put header styles here */ | |||
There was a problem hiding this comment.
This file currently contains only a placeholder comment. The task requires that "Split the App.js into components based on CSS blocks. CSS code should be split too." — please move or add the navigation-specific styles into this file so the Navigation component's styles are housed here (and remove the placeholder). See Navigation.jsx which imports this CSS for reference .
| <span className="welcome__text">Sticky Header!</span> | ||
| </section> | ||
| ); | ||
| } |
There was a problem hiding this comment.
This file has leftover template/scaffolding comments after the component. This violates the previous-review requirement: "As a minor cleanup, remove the leftover template comments in the component files." Remove those comments (they appear after the component closing brace) to clean up the codebase.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still critical, requirement-breaking issues to address before this can be considered complete. First, App.jsx does not import or render the Navigation component, even though the task explicitly requires: "Move the navigation block into its own Navigation component. Import and render the new Navigation component within App.jsx." — please add import Navigation from './components/Navigation/Navigation'; (or correct path) and include <Navigation /> in the returned JSX. Second, src/components/Navigation/Navigation.css currently contains only a placeholder comment; the requirement says CSS must be split per component, so move the navigation-specific styles from any shared CSS (e.g. Header.css/App.css) into this file and remove the placeholder. Third, several component files still contain leftover template comments (Article.jsx, Header.jsx, Welcome.jsx), which were explicitly requested to be removed in the previous review; delete those instructional comments at the end of each file to keep the source clean and follow the corrections request.
In addition, CI/component tests are failing due to CSS/layout mismatches (Header, Article, Welcome) and a likely incomplete Navigation styling/structure; you should run npm test locally, open the failing spec lines, and adjust the corresponding component CSS files so computed styles match what the tests expect (e.g., background color in Welcome.css, layout/width in Article.css, position/display in Header.css). Once you import/render Navigation from App.jsx, move the navigation styles into Navigation.css, remove all template comments, and fix the CSS so tests pass, your solution should fully meet the task and checklist requirements. You’ve done a solid job setting up the component structure and default exports; with these final fixes, the implementation will be in great shape.
✨ 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
| @@ -0,0 +1 @@ | |||
| /* Put header styles here */ | |||
There was a problem hiding this comment.
This file currently contains only a placeholder comment. The task requires that "Split the App.js into components based on CSS blocks. CSS code should be split too." — please move or add the navigation-specific styles into this file so the Navigation component's styles are housed here (and remove the placeholder). See Navigation.jsx which imports this CSS for reference .
| </p> | ||
| </article> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Remove the leftover template comments present after the component definition. The task's previous-review instruction said: "Remove the leftover template comments in the component files." Leaving those comments in the source is unnecessary noise — delete lines containing the template comments at the end of this file.
src/components/Header/Header.jsx
Outdated
| @@ -1,5 +1,14 @@ | |||
| // import a css file containig header styles | |||
| import './Header.css'; | |||
| import Navigation from '../Navigation/Navigation'; | |||
There was a problem hiding this comment.
This import references ../Navigation/Navigation. The task requires: "Move the navigation block into its own Navigation component. Import and render the new Navigation component within App.jsx." Create ./src/components/Navigation/Navigation.jsx and Navigation.css, ensure export default is used, and add the file to the repo.
src/components/Header/Header.jsx
Outdated
| return ( | ||
| <header className="header"> | ||
| <h1 className="header__title">Site Name</h1> | ||
| <Navigation /> |
There was a problem hiding this comment.
You're rendering <Navigation /> here inside Header. The description explicitly asks to "Import and render the new Navigation component within App.jsx." Move the render/import to App.jsx (or ensure App.jsx imports and renders the Navigation component) to satisfy that requirement and avoid duplicate placement.
| export default function Header() { | ||
| return ( | ||
| <header className="header"> | ||
| <h1 className="header__title">Site Name</h1> | ||
| <Navigation /> | ||
| </header> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Remove the leftover template comments at the end of this file (e.g. "Create a Header function returning the HTML of header block" and "Add a default export statement..."). These are leftover instructional comments from the template and should be removed before final submission.
| import './App.css'; | ||
| // Move each BEM block to a separate component (file) and import them here | ||
|
|
||
| import Header from './components/Header/Header'; |
There was a problem hiding this comment.
App.jsx does not import the Navigation component. The task states: "Move the navigation block into its own Navigation component. Import and render the new Navigation component within App.jsx." Add import Navigation from './components/Navigation/Navigation'; (or the correct relative path) near the other imports and then render <Navigation /> in the JSX.
| diam. | ||
| </p> | ||
| </article> | ||
| <Welcome /> |
There was a problem hiding this comment.
App.jsx does not render <Navigation /> inside the returned JSX. Insert <Navigation /> where the navigation should appear (for example, between and
| // Move each BEM block to a separate component (file) and import them here | ||
|
|
||
| import Header from './components/Header/Header'; | ||
| import Welcome from './components/Welcome/Welcome'; |
There was a problem hiding this comment.
This file (and repo snapshot) appears to violate checklist item #2: "create separate folder per component, where you could put all files(styles, components, and so on)" — a Navigation component folder/file is missing. Please add ./src/components/Navigation/Navigation.jsx and Navigation.css and commit them.
| export default function Welcome() { | ||
| return ( | ||
| <section className="welcome"> | ||
| <span className="welcome__text">Sticky Header!</span> | ||
| </section> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Remove leftover template comments at the end of the file. The task's previous-review corrections request: "Remove the leftover template comments in the component files." These comment lines (the instructional/template text after the component) should be deleted to keep the component file clean.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing while you refine a few remaining details.
You did a solid job setting up the component structure: each of Header, Article, Welcome, and Navigation lives in its own folder, uses export default, and has its own CSS file, which aligns well with the component-based CSS requirement [CHECKLIST ITEM #2]. The main issue to address next is that src/App.css still contains component-specific media queries for .header and .article; the requirement is that these responsive rules should be moved into src/components/Header/Header.css and src/components/Article/Article.css so styles stay fully component-scoped. There are also some small markup/CSS mismatches: for example, your Header.jsx h1 needs className="header__title" (or the CSS selector should be adjusted) so your .header__title styles actually apply, and your Article heading should either have a matching .article__title rule in Article.css or the class removed to keep things consistent. Finally, make sure to run npm test, then use the failing expectations (especially around layout, margins, and background colors) as a guide to fine-tune Header.css, Article.css, and Welcome.css until all style-related tests pass.
Overall, your structure and component setup are strong; with these focused CSS adjustments and a test run, your solution will be in great shape.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
https://ArtemNosachenko.github.io/react_decompose/