React decompose#4490
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is a structural HTML issue in your navigation that can break the task’s expectations and tests. In src/components/Navigation/Navigation.jsx (and similarly in the related files), you currently render a <nav className="navigation"> that contains another <nav className="navigation">; nested nav elements are invalid semantics and conflict with the requirement to have a single navigation block. To fix this, keep only one <nav className="navigation"> and place all your navigation links directly inside that element (no inner nav). Additionally, your JSX uses the navigation class on the nav, but Navigation.css only defines .navigation__link, so the container styles from the original App.css appear not to have been migrated; either add a .navigation rule here with the appropriate layout styles or remove the unused className if it’s not needed. There is also a usability issue where .navigation__link has opacity: 0.7 by default and the same opacity on :hover/:focus, so users see no visual feedback on interaction; consider increasing opacity or changing another property on hover/focus to make the links feel responsive. Once you correct the nested nav structure and align the navigation styles, the rest of your component decomposition looks solid, and you’re close to fully meeting the task requirements. Keep going – your overall setup with separate folders and default exports is good, and these fixes will bring your solution in line with both semantics and expected behavior.
✨ 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
| @@ -1 +1,30 @@ | |||
| /* Put article styles here */ | |||
| .article { | |||
There was a problem hiding this comment.
There is no .navigation selector defined in this CSS, but Navigation.jsx uses className="navigation" on the nav element. Given the requirement to "split App.js into components based on CSS blocks", you likely need to move the container styles for navigation into this file and add a .navigation rule, or remove the unused class if it's not needed.
| font-size: 140%; | ||
| max-width: 800px; | ||
| background: white; | ||
| box-shadow: rgba(0, 0, 0, 0.05) 0 3px 15px; |
There was a problem hiding this comment.
This violates the structural expectations of the navigation component: you have a <nav className="navigation"> wrapped around another <nav className="navigation">. Consider keeping a single <nav> and placing the links directly inside it; nested nav elements are not appropriate here and may break tests.
| efficitur, orci non vestibulum consequat, lectus turpis bibendum odio, in | ||
| efficitur leo felis sed justo. Fusce commodo iaculis orci, quis imperdiet | ||
| urna. Sed mollis facilisis lacus non condimentum. Nunc efficitur massa non |
There was a problem hiding this comment.
The default state of .navigation__link already uses opacity: 0.7, and the hover/focus state also sets opacity: 0.7, so there is no visible change on hover or focus. Consider making the interactive state visually distinct (e.g., higher opacity) so users can see feedback when they interact with the navigation links.
| padding: 1em; | ||
| font-size: 140%; | ||
| position: sticky; | ||
| top: 0; | ||
| left: 0; |
There was a problem hiding this comment.
You have a <nav> element nested inside another <nav>, which is not semantically correct and doesn't match a standard navigation component structure. Consider rendering a single nav with the navigation class that directly contains the anchor links.
| outline: none; | ||
| text-decoration: none; | ||
| opacity: 0.7; | ||
| padding: 0 0.5em; | ||
| color: black; |
There was a problem hiding this comment.
The current structure has a <nav> inside another <nav>, which is invalid semantic HTML and may break tests or accessibility expectations. Consider rendering a single <nav className="navigation"> that directly contains the links.
| <nav className="navigation"> | ||
| <nav className="navigation"> | ||
| <a href="#about" className="navigation__link"> | ||
| About | ||
| </a> | ||
| <a href="#services" className="navigation__link"> | ||
| Services | ||
| </a> | ||
| <a href="#contact" className="navigation__link"> | ||
| Contact | ||
| </a> | ||
| </nav> |
There was a problem hiding this comment.
This <nav> is nested inside another <nav> with the same class, which is not semantically correct and likely not what the task expects. You should have a single nav element that directly contains the navigation links.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because it fully meets the task requirements and passes all checklist items visible from the code. App.jsx is cleanly decomposed into Welcome, Header, Article, and Navigation, each living in its own folder with its own CSS, satisfying the “separate folder per component” requirement [CHECKLIST ITEM #2]. All components correctly use export default, and the imports/JSX structure are valid, so there are no functional or structural blockers. The previous critical issue with nested <nav> elements is resolved: Navigation.jsx now has a single <nav className="navigation"> with links inside, and .navigation plus the hover/focus opacity change are properly implemented in Navigation.css. I don’t see any remaining issues that would break functionality or violate the explicit task description, so no changes are required from a code perspective. Nice work aligning your structure and styles with the React decomposition goals for this task.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
DEMO LINK](https://balakhmei.github.io/react_decompose/)