solution 1#4444
Conversation
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 well-structured with proper folder organization and export default statements, but there's a critical CSS organization issue that violates task requirements. The .header class styles in Article.css (line 21) belong to the Header component, not Article. Per the task requirement that 'CSS code should be split too (along with the JS)', these styles must be moved to Header.css. Currently, Header.css only contains the media query and navigation styles, but the base .header layout rules are missing. Simply cut the .header CSS block from Article.css and paste it into Header.css before the navigation styles to complete the proper CSS separation per component.
✨ 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
| tellus, a facilisis magna. Proin sit amet viverra nibh, bibendum gravida | ||
| felis. Vivamus ut nunc id mauris posuere pellentesque. Praesent | ||
| tincidunt id odio id feugiat. | ||
| </p> |
There was a problem hiding this comment.
This .header CSS rule appears in Article.css but belongs to the Header component. CSS should be organized per component - move these styles to Header.css where the .header block class is actually used.
| <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 |
There was a problem hiding this comment.
The .navigation__link styles that go with the header/navigation are correctly in Header.css, but the corresponding base .header styles are incorrectly placed here in Article.css.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The request meets all requirements and is approved.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| @media (min-width: 500px) { | ||
| .header { | ||
| text-align: left; | ||
| display: flex; | ||
| justify-content: space-between; | ||
| } | ||
|
|
||
| .header__title { | ||
| font-weight: 600; | ||
| display: inline; | ||
| margin: 0; | ||
| padding: 0; | ||
| font-size: inherit; | ||
| } | ||
|
|
||
| .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.
CSS syntax error: .header__title selector (lines 20-26) and .navigation__link selectors (lines 28-41) are placed OUTSIDE the @media (min-width: 500px) closing brace on line 42. These styles should be INSIDE the media query block, nested after the .header { } rules and BEFORE the closing }. Currently they apply at all screen sizes instead of only when min-width: 500px.
2 participants
Solution 1