Decompose App into components#4428
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This submission has critical issues that prevent successful completion. The build fails due to a file extension error: src/components/Header/Navigation.js contains JSX syntax (like <nav> elements) but has a .js extension instead of .jsx. Vite/esbuild cannot parse JSX in .js files, which causes Header/Header.spec.jsx to fail with "The JSX syntax extension is not currently enabled." Additionally, the Navigation component exists in two locations (src/components/Header/Navigation.js and src/components/Navigation/Navigation.jsx), violating [CHECKLIST ITEM #2] which requires each component to have its own separate folder without duplication. You should remove the Navigation files from inside the Header folder and keep only the one in src/components/Navigation/. After removing the duplicate, update Header.jsx to import Navigation from ../Navigation/Navigation instead of ./Navigation. The Header.css file also contains .navigation__link styles (lines 21-34) that belong to the Navigation component. Move these styles to src/components/Navigation/Navigation.css so each component's CSS lives in its own folder. Finally, ensure Navigation is only rendered once—either directly in App.jsx or inside Header.jsx, not both. Please fix the .js to .jsx extension first, resolve the duplicates, and ensure consistent imports across all files.
✨ 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
| @media (min-width: 500px) { | ||
|
|
||
|
|
||
| .article { | ||
| margin: 3.5em auto 0; | ||
| padding: 2em; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| @media (min-width: 800px) { | ||
| .article { | ||
| margin: 3.5em auto; | ||
| } |
There was a problem hiding this comment.
The .navigation__link CSS styles (lines 21-34) are placed in Header.css but belong to the Navigation component. Per checklist item #2, each component should have its own styles in its folder. Consider moving these styles to Navigation.css.
|
|
||
| .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 CSS file contains .navigation__link styles (lines 21-34), but Navigation is a separate component with its own folder. According to checklist item #2, each component should have its CSS in its own folder. The navigation styles should only be in src/components/Navigation/Navigation.css.
| @@ -1,5 +1,65 @@ | |||
| // import a css file containig article styles | |||
|
|
|||
| import './Article.css'; | |||
There was a problem hiding this comment.
Header.jsx imports Navigation from ./Navigation (same folder), but App.jsx expects Navigation from ./components/Navigation/Navigation. The Navigation component should be in its own folder, not nested inside Header. Move Navigation out of the Header folder to src/components/Navigation/.
| </p> | ||
| <p className="article__paragraph"> | ||
| In ac nisi lacus. Fusce est dolor, tincidunt ut bibendum vitae, fermentum | ||
| ac quam. Aliquam pretium tristique nibh quis iaculis. In et cursus ex, eu | ||
| aliquet ex. Proin facilisis lacus sit amet sapien ultrices, ut vehicula | ||
| arcu lobortis. Vivamus mollis ipsum ut hendrerit molestie. Morbi lacinia, | ||
| sapien eu dictum dignissim, tellus tortor congue magna, sit amet bibendum | ||
| libero nisi id massa. | ||
| </p> | ||
| <p className="article__paragraph"> | ||
| Donec arcu elit, euismod vel lobortis eu, fringilla sit amet dolor. Cras | ||
| congue, massa nec sagittis mollis, dui felis ultrices magna, tincidunt | ||
| finibus lorem quam in sem. Morbi odio turpis, pulvinar sit amet vulputate | ||
| quis, ultricies eu libero. Donec ac maximus neque, nec maximus nibh. Morbi |
There was a problem hiding this comment.
Navigation CSS styles should not be in Header.css. The .navigation__link styles (lines 21-34) belong to the Navigation component, not the Header. These are duplicated in src/components/Navigation/Navigation.css and should only exist there.
| @@ -1,5 +1,65 @@ | |||
| // import a css file containig article styles | |||
There was a problem hiding this comment.
There are duplicate Navigation files: src/components/Header/Navigation.js and src/components/Navigation/Navigation.js, plus duplicate CSS files. The files inside Header folder should be removed, and Navigation should only exist in its own folder with consistent imports.
| import './App.css'; | ||
| // Move each BEM block to a separate component (file) and import them here | ||
| import Header from './components/Header/Header'; | ||
| import Navigation from './components/Navigation/Navigation'; |
There was a problem hiding this comment.
Inconsistent imports: App.jsx imports Navigation from ./components/Navigation/Navigation while Header.jsx imports from ./Navigation (inside Header folder). Choose one location and use it consistently.
| import './App.css'; | ||
| // Move each BEM block to a separate component (file) and import them here | ||
| import Header from './components/Header/Header'; | ||
| import Navigation from './components/Navigation/Navigation'; |
There was a problem hiding this comment.
Header imports Navigation from ./Navigation which is inside the Header folder. If Navigation should be a separate component (as implied by App.jsx importing it directly), it shouldn't be nested inside Header.
| @@ -1,5 +1,16 @@ | |||
| // import a css file containig header styles | |||
| import Navigation from './Navigation'; | |||
There was a problem hiding this comment.
Duplicate Navigation component exists in two locations. Header.jsx imports from ./Navigation (inside Header folder, line 2), but App.jsx imports from ./components/Navigation/Navigation. Choose one location - the description suggests creating a separate Navigation folder. Remove the Navigation files from inside the Header folder.
| // import a css file containig header styles | ||
| import Navigation from './Navigation'; | ||
| import './Header.css'; | ||
|
|
||
| // 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 | ||
|
|
||
| const Header = () => ( | ||
| <header className="header"> | ||
| <h1 className="header__title">Site Name</h1> | ||
| <Navigation /> | ||
| </header> | ||
| ); | ||
|
|
There was a problem hiding this comment.
Duplicate CSS file: Navigation.css exists in both src/components/Header/ and src/components/Navigation/. Only one copy should remain in the separate Navigation folder.
| @@ -0,0 +1,15 @@ | |||
|
|
|||
| .navigation__link { | |||
There was a problem hiding this comment.
Header.jsx imports Navigation from ./Navigation (inside Header folder) instead of from the Navigation component's own folder. Since App.jsx imports Navigation from ./components/Navigation/Navigation, Header.jsx should import from ../Navigation/Navigation to use the same component.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The provided response contains markdown text and instructions instead of valid JSON. The JSON must strictly adhere to the schema with 'decision' as either 'APPROVE' or 'REQUEST_CHANGES' and a 'reason' string. Fix the content to return only valid JSON matching the expected 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
| /* Put article styles here */ | ||
|
|
||
| .article { | ||
| margin: 5em auto 0; |
There was a problem hiding this comment.
This file contains JSX syntax (like <nav> elements) but has a .js extension. Vite/esbuild cannot parse JSX in .js files, which causes tests to fail. Rename this file to Navigation.jsx.
| @media (min-width: 500px) { | ||
|
|
||
|
|
||
| .article { | ||
| margin: 3.5em auto 0; | ||
| padding: 2em; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| @media (min-width: 800px) { | ||
| .article { | ||
| margin: 3.5em auto; | ||
| } |
There was a problem hiding this comment.
The .navigation__link styles (lines 21-34) belong to the Navigation component, not Header. Move these CSS rules to src/components/Navigation/Navigation.css and remove them from Header.css to follow the requirement that CSS should be split per component.
| const App = () => ( | ||
| <> | ||
| <Header /> | ||
| <Navigation /> |
There was a problem hiding this comment.
Navigation is rendered twice - once in App.jsx (line 9) and once inside Header.jsx (line 7). Remove one of them. Based on the previous review feedback, remove the <Navigation /> from App.jsx since Navigation is already inside Header.
| <Header /> | ||
| <Navigation /> |
There was a problem hiding this comment.
Navigation is rendered inside Header, so it should not also be rendered directly in App.jsx. This is duplicate rendering. Remove line 2 (Navigation import) and line 9 () from App.jsx.
| // import a css file containig article styles | ||
|
|
||
| import './Article.css'; | ||
| // Create an Article function returning the HTML of article block |
There was a problem hiding this comment.
This file contains JSX syntax (<nav> elements) but has a .js extension. Vite/esbuild cannot parse JSX in .js files. Rename to .jsx to fix the build error.
|
|
||
| .navigation__link { | ||
| display: inline-block; |
There was a problem hiding this comment.
This file contains JSX syntax (<nav> elements) but has a .js extension. Vite/esbuild cannot parse JSX in .js files, which causes the error "The JSX syntax extension is not currently enabled". Rename this file to Navigation.jsx.
| opacity: 0.7; | ||
| padding: 0 0.5em; | ||
| color: black; | ||
| transition: opacity 0.2s ease-in-out; |
There was a problem hiding this comment.
Navigation is rendered twice: once directly in App.jsx (line 9) and once inside Header.jsx (line 12). Remove one of them. Since Header already includes Navigation, remove the <Navigation /> and its import from App.jsx.
| @@ -0,0 +1,15 @@ | |||
|
|
|||
| .navigation__link { | |||
There was a problem hiding this comment.
After renaming Navigation.js to Navigation.jsx, update this import path to '../Navigation/Navigation' or just '../Navigation/Navigation.jsx' to correctly reference the renamed file.
| @@ -0,0 +1,17 @@ | |||
| import './Navigation.css'; | |||
|
|
|||
| const Navigation = () => ( | |||
There was a problem hiding this comment.
Navigation.js contains JSX syntax (<nav> elements on lines 3-13) but has a .js extension. Vite/esbuild cannot parse JSX in .js files. This must be renamed to Navigation.jsx.
| @@ -0,0 +1,17 @@ | |||
| import './Navigation.css'; | |||
|
|
|||
There was a problem hiding this comment.
Header.jsx imports Navigation from './Navigation' (line 2), but if the file is renamed to .jsx, the import path should be updated to '../Navigation/Navigation' to reference the component in its own folder per CHECKLIST ITEM #2.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
|
mais uma revisao |
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your component decomposition is on the right track! The folder structure (Header, Navigation, Welcome, Article) and use of export default meet the requirements. However, there are two functional issues to address: Navigation renders twice - it's imported in both Header.jsx (line 14) and App.jsx (line 9), which will display navigation links twice on the page (remove one). Also, CSS duplication - the .navigation__link styles are in both Header.css (lines 21-34) and Navigation.css; since CSS should be split along with components, remove these styles from Header.css and keep them only in Navigation.css. These fixes will ensure your implementation is production-ready. Great job on the overall structure - keep practicing component composition!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| <h2 className="article__title">Headline</h2> | ||
| <p className="article__paragraph"> |
There was a problem hiding this comment.
Navigation is imported and rendered inside Header component (line 5 of Header.jsx). Including <Navigation /> directly in App.jsx line 9 will cause Navigation to appear twice on the page. Either remove Navigation from Header.jsx or remove it from App.jsx.
|
|
||
| .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 violates the component decomposition principle. The .navigation__link styles (lines 21-34) are already defined in Navigation.css and should not be duplicated in Header.css. Each component should own only its own styles. Consider removing these navigation-related styles from Header.css.
| @media (min-width: 500px) { | ||
|
|
||
|
|
||
| .article { | ||
| margin: 3.5em auto 0; | ||
| padding: 2em; | ||
| } | ||
| } | ||
|
|
||
|
|
||
| @media (min-width: 800px) { | ||
| .article { | ||
| margin: 3.5em auto; | ||
| } |
There was a problem hiding this comment.
The .navigation__link styles (lines 21-34) are duplicated in Header.css but Navigation is already its own component with its own CSS file. This CSS should only exist in Navigation.css. The task requires splitting CSS along with JavaScript components.
| <a className="navigation__link" href="#services"> | ||
| Services |
There was a problem hiding this comment.
Navigation is rendered twice - once inside Header component (line 14) and once directly in App.jsx (line 9). This will cause Navigation to display twice on the page. Consider removing <Navigation /> from either Header.jsx or App.jsx depending on the intended structure.
|
|
||
| // 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.
Navigation is rendered twice: once inside Header component (Header.jsx line 14) and once directly in App.jsx line 9. This will display navigation links twice on the page. Since Navigation is already included inside Header, remove <Navigation /> from App.jsx line 9.
2 participants
DEMO LINK: https://sistsayao-hue.github.io/react_decompose/