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 failing tests and one core requirement about splitting CSS is not fully met. In src/App.css, the responsive @media rules that style .header and .article (near the bottom of the file) are still defined globally instead of being moved into src/components/Header/Header.css and src/components/Article/Article.css, which violates the requirement to split CSS along with components. In src/components/Article/Article.spec.jsx around line 28, the test should have styles added with media expects a responsive style change (e.g. a font-size of 22.4px on the tested element), but the actual computed value remains 44.8px, so you need to add or correct the @media rule in Article.css so that at the tested viewport the property matches the expected value. Similarly, in src/components/Header/Header.spec.jsx around line 35, the test should have styles added with media expects the target element to switch its display from flex to block under a media query; update Header.css so that at the viewport used in the spec, the selector under test has display: block while keeping the default (likely display: flex) for the other breakpoint. After moving the .header and .article media styles out of App.css and into their respective component CSS files, double-check that there are no duplicate/conflicting rules and that the selectors match the JSX structure. As minor follow‑ups, run npm test locally until all tests pass, ensure all files are committed to git, and keep using Prettier/format-on-save so formatting stays consistent. Overall your component decomposition and basic styling are solid; once you fix these responsive CSS issues and test failures, your solution will fully meet the task requirements.
✨ 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
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 two required responsive behaviors are still incorrect and will keep the tests failing. The .article element currently has a base font-size of 140%, but src/components/Article/Article.css does not include a media query that reduces this at the tested viewport, so the computed size stays around 44.8px instead of the expected 22.4px; add a media rule for the viewport used in Article.spec.jsx (for example a max-width breakpoint) that sets .article { font-size: 70%; } or an explicit 22.4px. The header also does not meet the responsive requirement: in src/components/Header/Header.css the default .header is not explicitly display: flex, and the media rule does not switch it to display: block at the smaller tested viewport; set the base .header rule to display: flex and add or adjust a media query (e.g. @media (max-width: 499px)) so that .header { display: block; } there.
These two issues are critical because the task explicitly requires them and the corresponding Jest tests (Header and Article media tests) fail when the styles are missing or inverted. After you update the media queries, double-check that App.css contains only global/reset styles and no conflicting .header or .article rules, then run npm test until all specs pass. The overall component structure, decomposition, and most CSS separation are well done, so once you correct these responsive rules your solution should fully meet the task requirements.
✨ 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
| const Article = () => ( | ||
| <article className="article"> | ||
| <h2 className="article__title">Headline</h2> | ||
| <p className="article__paragraph"> |
There was a problem hiding this comment.
The .article font-size is set here but there is no media rule that changes it to the value required by the tests. The task requires: "In src/components/Article/Article.css, add or correct the @media rule so that ... the tested element’s font-size changes from 44.8px to the expected 22.4px." Add a media query targeting the viewport used in the spec and set .article { font-size: 70%; } (or explicit 22.4px) so the computed size matches the test.
| 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> |
There was a problem hiding this comment.
There are media queries here that adjust margin/padding but none adjust font-size. Per the Implementation Details you must add or correct the @media rule so the tested element’s font-size changes at the viewport used in Article.spec.jsx. Consider adding the font-size change inside the appropriate @media block (or create a new one that matches the spec's viewport).
| 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 | ||
| rhoncus convallis urna, accumsan porta lorem hendrerit in. Cras eget nisl |
There was a problem hiding this comment.
The @media (min-width: 800px) block currently only changes margin. Double-check which breakpoint the test uses and ensure there are no conflicting rules that keep the font-size at 44.8px. If the test expects 22.4px at a specific viewport, the corresponding media rule must explicitly set the font-size there.
| /* Put article styles here */ | ||
| .article { | ||
| max-width: 800px; | ||
| margin: 5em auto 0; |
There was a problem hiding this comment.
Header’s responsive behaviour still doesn't match the task requirement. The tests expect the header to have display: flex by default and to switch to display: block at the tested viewport. Currently your Header.css sets display: flex only inside a @media (min-width: 500px) block (so the default is not flex). Update Header.css so the default .header is display: flex and add a media rule (matching the spec viewport) that sets .header { display: block } at that breakpoint.
| margin: 5em auto 0; | ||
| padding: 1em; | ||
|
|
||
| font-size: 140%; |
There was a problem hiding this comment.
The .article element sets font-size: 140% but there is no media rule that adjusts font-size. The test should have styles added with media expects the article’s computed font-size to change from 44.8px to 22.4px at the spec viewport. Add or correct a media query in Article.css that halves the .article font-size at the tested breakpoint (e.g. set .article { font-size: 50% } inside the appropriate @media), so the computed value becomes 22.4px.
| import Header from './components/Header/Header'; | ||
| import Welcome from './components/Welcome/Welcome'; | ||
| import Article from './components/Article/Article'; | ||
|
|
There was a problem hiding this comment.
.article currently sets font-size: 140% which yields the larger computed value seen in the tests (44.8px). The task requires a responsive change so the tested viewport produces 22.4px. Add or adjust a media rule to change .article font-size at the breakpoint used by the spec (for example, set .article { font-size: 70%; } inside the appropriate @media so computed size becomes 22.4px).
| @@ -0,0 +1,17 @@ | |||
| import './Navigation.css'; | |||
There was a problem hiding this comment.
The .header block doesn't set a default display. The task requires that .header has display: flex at one breakpoint and switches to display: block at the tested viewport. Add display: flex to the default .header rules so the default state is flex.
| const Navigation = () => ( | ||
| <nav className="navigation"> | ||
| <a className="navigation__link" href="#about"> | ||
| About |
There was a problem hiding this comment.
.article currently sets font-size: 140% which produces the larger computed value (~44.8px). The Article.spec test requires a responsive change so the computed font-size becomes ~22.4px at the tested viewport. Add or adjust a media rule in Article.css that reduces .article font-size at that viewport (for example a rule that halves the font-size or sets it to the explicit value expected by the test).
| .header { | ||
| position: sticky; | ||
| top: 0; | ||
| right: 0; | ||
| left: 0; | ||
|
|
||
| width: 100%; | ||
| padding: 1em; | ||
|
|
||
| font-size: 140%; | ||
| text-align: center; | ||
|
|
||
| transition: opacity 0.2s ease-in-out; | ||
| } |
There was a problem hiding this comment.
Add an explicit default display for the header. The tests expect .header to be display: flex by default; currently display: flex is only defined inside a media query so the default falls back to block implicitly. Set display: flex in the root .header rule.
| @media (min-width: 500px) { | ||
| .header { | ||
| display: flex; | ||
| justify-content: space-between; | ||
| text-align: left; | ||
| } | ||
| } |
There was a problem hiding this comment.
Adjust the media query so it implements the tested breakpoint behaviour: the header should switch to display: block at the viewport used in the spec. For example, add @media (max-width: 499px) { .header { display: block; } } (or change the current media rule to set display: block at the tested breakpoint) and keep the larger-viewport display: flex intact.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing while you practice these responsive CSS patterns. You’ve done a solid job on the component structure: each component lives in its own folder, uses export default, and App.css is kept for global/reset styles only. The two main issues to revisit are responsive styles: in src/components/Article/Article.css the .article currently has font-size: 140% but is missing a small-viewport rule like @media (max-width: 499px) { .article { font-size: 70%; } } (or an explicit font-size: 22.4px;), which is why the Article media test is failing. In src/components/Header/Header.css, the base .header rule needs to explicitly be display: flex;, and then you should add a small-viewport media query such as @media (max-width: 499px) { .header { display: block; } } instead of only setting display: flex inside @media (min-width: 500px), which currently inverts the required behavior. After adjusting those two CSS areas, run npm test again to confirm the Cypress media tests pass, and double-check that all files are committed as expected [CHECKLIST ITEM #3]. Overall, your decomposition and use of per-component CSS show good understanding, and tightening these responsive details will bring the implementation fully in line with the requirements.
✨ 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
| const Article = () => ( | ||
| <article className="article"> | ||
| <h2 className="article__title">Headline</h2> | ||
| <p className="article__paragraph"> |
There was a problem hiding this comment.
The .article element currently has font-size: 140% here. The task requires a media rule for the smaller tested viewport that reduces this size (for example: @media (max-width: 499px) { .article { font-size: 70%; } } or an explicit 22.4px) so the computed font-size matches the test expectation.
| 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 | ||
| rhoncus convallis urna, accumsan porta lorem hendrerit in. Cras eget nisl |
There was a problem hiding this comment.
This file only contains min-width media queries. You must add a media query for the smaller viewport (e.g. @media (max-width: 499px) { .article { font-size: 70%; } }) to reduce the .article font-size at small widths — the absence of that rule is what makes Article.spec.jsx fail.
| margin: 5em auto 0; | ||
| padding: 1em; | ||
|
|
||
| font-size: 140%; |
There was a problem hiding this comment.
HIGH: The .article element has a base font-size: 140% but there is no media query that reduces it for the smaller tested viewport. Add a media rule (for example @media (max-width: 499px) { .article { font-size: 70%; } } or set font-size: 22.4px) in src/components/Article/Article.css so tests expecting the reduced computed size will pass.
|
|
||
| html { | ||
| font-family: 'Source Sans Pro', 'Open Sans', Roboto, Arial, sans-serif; | ||
| font-size: 10pt; |
There was a problem hiding this comment.
Header component is used here, but src/components/Header/Header.css does not follow the required rules: the base .header rule must explicitly be display: flex, and you must add/adjust a media query for the smaller tested viewport (for example @media (max-width: 499px)) so that .header { display: block; } inside that media rule. Currently the stylesheet uses @media (min-width: 500px) { .header { display: flex; } }, which is inverted relative to the spec and the tests.
| import Header from './components/Header/Header'; | ||
| import Welcome from './components/Welcome/Welcome'; | ||
| import Article from './components/Article/Article'; | ||
|
|
There was a problem hiding this comment.
Article currently sets font-size: 140% but there is no media query to reduce it at the tested smaller viewport. Per the task: add a media rule (for the viewport used in Article.spec.jsx, e.g. @media (max-width: 499px)) that sets .article { font-size: 70%; } or an explicit 22.4px so the computed size matches the expected ~22.4px.
| @@ -1,5 +1,11 @@ | |||
| // import a css file containig header styles | |||
| import './Header.css'; | |||
There was a problem hiding this comment.
The base .header rule does not include an explicit display. The task requires the base rule to explicitly be display: flex (so the default layout is flex for larger viewports). Add display: flex; to this rule.
| @@ -1 +1,32 @@ | |||
| /* Put header styles here */ | |||
| .header { | |||
There was a problem hiding this comment.
The base .header rule does not explicitly set display. The task requires the default .header to be display: flex. Add display: flex; to this rule so larger-layout defaults are correct and so you can switch to display: block in a small-viewport media query.
| @media (min-width: 500px) { | ||
| .header { | ||
| display: flex; |
There was a problem hiding this comment.
This media rule uses @media (min-width: 500px) to set .header { display: flex; }. That is inverted relative to the requirement. Instead either move display: flex to the base .header rule and add a small-viewport media query (e.g. @media (max-width: 499px) { .header { display: block; } }), or change this rule to a max-width media query that sets display: block for small viewports. The tests expect default flex and small-viewport block.
| const Welcome = () => ( | ||
| <section className="welcome"> | ||
| <span className="welcome__text">Sticky Header!</span> | ||
| </section> |
There was a problem hiding this comment.
The .article rule currently sets font-size: 140%. The task requires adding a small-viewport media query so the computed font-size at the tested viewport is around ~22.4px. Add a rule such as @media (max-width: 499px) { .article { font-size: 70%; } } or set font-size: 22.4px.
| @@ -1,5 +1,9 @@ | |||
| // import a css file containig welcome styles | |||
| import './Welcome.css'; | |||
There was a problem hiding this comment.
The base .header rule does not explicitly set display. Per the requirements, set the default layout: add display: flex; to the base .header rule so the default state is flex.
3 participants
[DEMO LINK] (https://ikseloi.github.io/react_decompose/)