Skip to content

React decompose#4490

Open
balakhmei wants to merge 3 commits into
mate-academy:masterfrom
balakhmei:develop
Open

React decompose#4490
balakhmei wants to merge 3 commits into
mate-academy:masterfrom
balakhmei:develop

Conversation

@balakhmei

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

@@ -1 +1,30 @@
/* Put article styles here */
.article {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +4 to +7
font-size: 140%;
max-width: 800px;
background: white;
box-shadow: rgba(0, 0, 0, 0.05) 0 3px 15px;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +11 to +13
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +3 to +7
padding: 1em;
font-size: 140%;
position: sticky;
top: 0;
left: 0;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +3 to +7
outline: none;
text-decoration: none;
opacity: 0.7;
padding: 0 0.5em;
color: black;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +5 to +16
<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>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! ✨

Footnotes

  1. Rate AI review example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants