Develop#4475
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still critical issues that block the task requirements from being fully met. First, the navigation is not extracted into its own Navigation component: the <nav className="navigation">...</nav> markup and related .navigation__link styles are still inside Header.jsx / Header.css, which violates the requirement to move navigation into a dedicated Navigation component/folder with its own CSS ([CHECKLIST ITEM #2]). Second, there are CSS syntax errors: in both Article.css and Header.css (and similar media blocks), the @media rules end with }; instead of }, which makes the CSS invalid and can break parsing and tests. Also in Article.jsx, you use className="article__title" but there is no corresponding .article__title rule in Article.css, which means styles are incomplete for that BEM element.
To fix this:
- Create a
src/components/Navigation/Navigation.jsxandNavigation.css, move the<nav>markup out ofHeader.jsxinto this new component, and import/use<Navigation />insideHeader. Move.navigation,.navigation__link, and related hover/focus styles fromHeader.csstoNavigation.cssto keep styles split per component/BEM block. - In all CSS files, change media query endings from
};to}(for example, inHeader.cssat the end of@media (min-width: 500px)and inArticle.cssat the end of@media (min-width: 800px)) so the CSS parses correctly. - Add a
.article__titleselector toArticle.csswith appropriate typography (font-size, weight, margin, etc.) so it matches theclassNameused inArticle.jsx.
Once you address these points, your overall decomposition and styling structure will align well with the task, and the rest of your React setup looks solid—nice work so far, just polish these last critical pieces.
✨ 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
| molestie. Morbi lacinia, sapien eu dictum dignissim, tellus tortor | ||
| congue magna, sit amet bibendum libero nisi id massa. | ||
| </p> | ||
| <p className="article__paragraph"> |
There was a problem hiding this comment.
The media query block ends with }; which is invalid in plain CSS. According to the task, styles should be correctly split per component, so you should close this block with just } (without the semicolon) to avoid CSS parse errors.
| function Article() { | ||
| return ( | ||
| <article className="article"> | ||
| <h2 className="article__title">Headline</h2> |
There was a problem hiding this comment.
Article.jsx uses a className="article__title", but there is no corresponding .article__title rule in this CSS file. Since the description requires splitting CSS per component and keeping styles aligned with BEM blocks, consider adding a style block for .article__title here.
| display: flex; | ||
| justify-content: space-between; | ||
| } | ||
| }; |
There was a problem hiding this comment.
The media query ends with };, which is invalid CSS. You should close the @media (min-width: 500px) rule with just } to avoid parse errors.
| /* stylelint-disable-next-line order/properties-order */ | ||
| transition: opacity 0.2s ease-in-out; | ||
| /* stylelint-disable-next-line order/properties-order */ | ||
| text-align: center; | ||
| } | ||
|
|
||
| .header__title { | ||
| font-weight: 600; | ||
| /* stylelint-disable-next-line order/properties-order */ | ||
| display: inline; | ||
| /* stylelint-disable-next-line order/properties-order */ |
There was a problem hiding this comment.
Navigation markup (<nav className="navigation">...) is still embedded directly in Header. The task requires moving navigation into a separate Navigation component, so you should extract this block into its own component and import it here.
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
I’m requesting changes because there are still a couple of missing CSS rules for class names that are already used in your JSX, which violates the task requirement to complete styles for existing class names and leaves parts of the UI incompletely styled.
Critical issues to fix:
- In
src/components/Article/Article.jsx, the heading usesclassName="article__title", butsrc/components/Article/Article.cssdoes not define a.article__titlerule. This breaks the requirement that every BEM element used in the markup has corresponding styles. Add a.article__titleselector with appropriate typography (e.g., font-size, font-weight, margin) so the article title is styled consistently. - In
src/components/Navigation/Navigation.jsx, the<nav>element usesclassName="navigation", butsrc/components/Navigation/Navigation.csscurrently only defines.navigation__link. To properly style the BEM block and satisfy the “split styles per component/BEM block” requirement, add a.navigationselector that handles layout/alignment of the navigation links (for example, flex layout, spacing, or inline display).
These are functional styling requirements tied directly to the task description, not just cosmetic improvements, so they need to be addressed before the solution can be considered complete. Aside from these missing rules, your overall component decomposition, folder structure, and media query syntax look good, and you’re very close to finishing this task successfully.
✨ 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
| .article { | ||
| margin: 3.5em auto; | ||
| } | ||
| } |
There was a problem hiding this comment.
This file is missing a .article__title rule, even though Article.jsx uses className="article__title"; this violates the requirement to "Complete styles for existing class names" by adding .article__title with appropriate typography.
| } | ||
|
|
||
| .header__title { | ||
| font-weight: 600; |
There was a problem hiding this comment.
Navigation.jsx uses className="navigation" on the <nav>, but Navigation.css only defines .navigation__link; to match the requirement of splitting styles per component/BEM block and completing styles for existing class names, add a .navigation selector here with appropriate layout styles.
| /* Put welcome styles here */ | ||
|
|
||
| .welcome { | ||
| height: 100vh; | ||
| background: coral; | ||
| } | ||
|
|
||
| .welcome__text { | ||
| margin: 0; | ||
| /* stylelint-disable-next-line order/properties-order */ | ||
| font-size: 400%; | ||
| text-align: center; | ||
| /* stylelint-disable-next-line order/properties-order */ | ||
| line-height: 1; | ||
| /* stylelint-disable-next-line order/properties-order */ | ||
| padding-top: calc(50vh - 20pt); | ||
| /* stylelint-disable-next-line order/properties-order */ | ||
| display: block; | ||
| /* stylelint-disable-next-line order/properties-order */ | ||
| font-weight: 700; |
There was a problem hiding this comment.
Navigation.jsx uses className="navigation" on the <nav> element, but there is no corresponding .navigation rule here; to fully "complete styles for existing class names" and keep styles split per BEM block, add a .navigation selector (e.g., to control layout/alignment of the links).
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
Your solution is approved — the components are well decomposed, each lives in its own folder, and all BEM class names used in JSX have matching styles in their respective .css files, which aligns nicely with the task and [CHECKLIST ITEM #2]. Welcome, Header, Article, and Navigation are all cleanly implemented as function components with export default, and the previously missing styles like article__title and navigation have been correctly added. Structurally, App.jsx now plays the role of a simple composer, which is a good React pattern and keeps each component focused on its own responsibility. The main remaining things are process-related: make sure you’ve run npm test, set up Prettier/VSCode formatting as required, and committed all new/changed files to git ([CHECKLIST ITEM #3]). Overall, this is a solid, well-organized solution — nice work progressing the decomposition and styling to match the requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
DEMO LINK