task completed by mirlik · Pull Request #4487 · mate-academy/react_decompose

mirlik · 2026-07-07T13:33:53Z

mateacademy-ai-mentor

Review Summary

I am requesting changes because there are still a couple of critical requirement gaps that affect correctness of the task implementation. First, the task explicitly requires splitting CSS by components, but the responsive media queries for .header and .article are still in src/App.css instead of in src/components/Header/Header.scss and src/components/Article/Article.scss; this means Header and Article do not fully own their styles and the key requirement is not yet met. Second, the navigation styles are effectively broken because the JSX uses className="nav" on the <nav> element while Nav.scss defines a .navigation block and nested &__link, so the compiled selectors (like .navigation__link) don’t match the actual DOM classes and the nav links won’t be styled as intended.

The critical issues to fix are:

Move the .header media query from App.css into Header.scss, and the .article media query from App.css into Article.scss, so each component’s SCSS contains all of its base and responsive styles.
Align the navigation class names: either change <nav className="nav"> in Nav.jsx to <nav className="navigation">, or update Nav.scss to use .nav { &__link { ... } }, ensuring that the BEM block name used in JSX matches the one in SCSS.
Ensure the root App container’s className="app" either has a corresponding .app rule in App.css or is renamed/removed so you’re not using an unused class.

Aside from these issues, your component decomposition, default exports, and general structure look good and align well with the task; once you address the CSS placement and nav class mismatch, the solution should be in great shape.

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-07-07T13:35:42Z

+  @media (min-width: 500px) {
+    .header {
+      text-align: left;
+      display: flex;
+      justify-content: space-between;
+    }
  }


The media query for .header is a component-specific style and should be moved into src/components/Header/Header.scss to satisfy the requirement that CSS be split along with the component decomposition.

mateacademy-ai-mentor · 2026-07-07T13:35:42Z

+  @media (min-width: 800px) {
+    .article {
+      margin: 3.5em auto;
+    }
  }


The media query for .article is article-specific and should be moved into src/components/Article/Article.scss so that each component owns its styles instead of keeping them in App.css.

mateacademy-ai-mentor · 2026-07-07T13:35:42Z

+import './Header.scss';
+import Nav from '../Nav/Nav';

 // Create a Header function returning the HTML of header block
+function Header() {
+  return (
+    <header className="header">
+      <h1 className="header__title">Site Name</h1>
+      <Nav />
+    </header>
+  );
+}

 // Add a default export statement for Header component to use it in the other files
+export default Header;


The header component is correct structurally and uses a default export, but note that any header-specific responsive styles should live in Header.scss rather than App.css; currently the media query exists globally, which conflicts with the “split CSS code too” requirement.

mateacademy-ai-mentor · 2026-07-07T13:35:42Z

+  padding: 1em;
+  font-size: 140%;
+  position: sticky;


The Nav component uses className="nav" on the <nav> element, but Nav.scss only defines .navigation { ... }; as a result, the nav container has no matching styles. Update either the className to navigation or the SCSS to include a .nav block.

mateacademy-ai-mentor · 2026-07-07T13:35:42Z

+/* Put header styles here */
+.header {
+  display: flex;
+  width: 100%;
+  padding: 1em;
+  font-size: 140%;
+  position: sticky;
+  top: 0;
+  left: 0;
+  right: 0;
+  transition: opacity 0.2s ease-in-out;
+  text-align: center;
+
+
+  &__title {
+    font-weight: 600;


Each <a> tag uses className="navigation__link", but the SCSS uses nested &__link under .navigation, which compiles to .navigation__link; since there is no .navigation class on any element, the BEM structure is inconsistent and the styles may not be logically grouped. Consider adding a .navigation class on the nav or flattening the selector to .navigation__link directly.

mateacademy-ai-mentor · 2026-07-07T13:35:42Z

+// Create a Header function returning the HTML of header block
+function Nav() {
+  return (
+    <nav className="nav">


The nav element uses className="nav", but the SCSS is written for a .navigation block (.navigation { &__link { ... } }), so the compiled .navigation__link selector does not match any DOM element; align the block class name in JSX and SCSS so navigation styles apply correctly.

mateacademy-ai-mentor · 2026-07-07T13:35:42Z

@@ -0,0 +1,21 @@
+// import a css file containig header styles


This SCSS defines a .navigation block, but the JSX uses className="nav"; to satisfy the BEM structure and ensure styles are applied, the block name here should match the class used on the <nav> element (or vice versa).

mateacademy-ai-mentor · 2026-07-07T13:35:42Z

+    outline: none;
+    text-decoration: none;
+    opacity: 0.7;
+    padding: 0 0.5em;


The container uses className="nav", but Nav.scss defines styles only for .navigation and .navigation__link. Either update this to className="navigation" or add a .nav selector in your styles so the navigation block is properly styled and consistent with your BEM naming.

mateacademy-ai-mentor · 2026-07-07T13:35:42Z

+.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;
+
+    &:hover,
+    &:focus {
+      opacity: 1;
+    }
+  }
+}


Nav.scss defines a .navigation block, but Nav.jsx never uses that class; only .navigation__link will be styled. Align your JSX and SCSS by using the same block name on the <nav> element.

mateacademy-ai-mentor

Review Summary

I am requesting changes because there is still a critical functional issue with the navigation styling and one remaining requirement gap with the root App class. The nav block uses className="nav" in multiple components (e.g., Navigation.jsx, Header.jsx, Welcome.jsx), while Navigation.scss defines the BEM block as .navigation { &__link { ... } }; this mismatch means the .navigation styles never apply to the <nav> element, and the nested .navigation__link selector will not match the links as intended, violating the explicit requirement to align navigation class names between JSX and SCSS. To fix this, either change each <nav className="nav"> to <nav className="navigation"> or change your SCSS root block to .nav { &__link { ... } } so the compiled selectors match the DOM. In addition, your root App container still uses className="app" in App.jsx while App.css does not define a .app rule, which breaks the requirement that this class must either be styled or removed/renamed to avoid unused classes; you can resolve this by adding a .app selector with your main layout styles or by removing/renaming the className on the root element. Apart from these issues, your component decomposition, SCSS co-location, and media query placement look good, so once you align the nav classes and handle the app class, your solution should fully meet the task requirements.

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-07-07T13:57:12Z

-          diam.
-        </p>
-      </article>
+      <Welcome />


The requirements still expect the root App container’s className="app" to either have a matching .app rule in App.css or be renamed/removed; right now there is no .app style, so this violates the medium-priority requirement about avoiding unused classes.

mateacademy-ai-mentor · 2026-07-07T13:57:13Z

+    <article className="article">
+      <h2 className="article__title">Headline</h2>
+      <p className="article__paragraph">
+        In elementum lorem eget est euismod ornare. Phasellus sit amet


The root element uses className="app", but there is no corresponding .app rule in App.css. This does not meet the generalized requirement: "Ensure the root App container’s className="app" either has a corresponding .app rule in App.css, or is renamed/removed so you are not using an unused class." Consider adding a .app style or removing/renaming this class.

mateacademy-ai-mentor · 2026-07-07T13:57:13Z

+import './Article.scss';

 // Create an Article function returning the HTML of article block
+function Article() {


The nav container is rendered with className="nav" while the links use className="navigation__link". Navigation.scss defines .navigation { &__link { ... } }, so the compiled selector .navigation__link does not match any element inside a .navigation block. This violates the HIGH requirement to "Align the navigation class names so the JSX and SCSS block names match" and will prevent styles from being applied correctly. Either change this to <nav className="navigation"> or adjust the SCSS block to .nav { &__link { ... } }.

mateacademy-ai-mentor · 2026-07-07T13:57:13Z

+  background: white;
+  box-shadow: rgba(0, 0, 0, 0.05) 0 3px 15px;
+  margin-bottom: 78.4px;
+  padding-left: 44.8px;


The root App component uses className="app", but there is no corresponding .app rule in App.css; this violates the requirement to ensure the root container’s className either has a matching style or is not used. Consider either adding a .app style in App.css or removing/renaming this class.

mateacademy-ai-mentor · 2026-07-07T13:57:13Z

+.article {
+  // margin: 5em auto 0;
+  // padding: 1em;
+  font-size: 140%;


The <nav> element uses className="nav" while the SCSS defines a .navigation block, so the BEM block name in JSX does not match the SCSS block name. This violates the requirement to align navigation class names so JSX and SCSS block names match; either change this to className="navigation" or update your SCSS to .nav { &__link { ... } }.

mateacademy-ai-mentor · 2026-07-07T13:57:13Z

 // Create a Header function returning the HTML of header block
+function Header() {
+  return (
+    <header className="header">


The nav element uses className="nav" while your SCSS defines .navigation as the block, producing selectors like .navigation__link. This violates the requirement: "Align the navigation class names so the JSX and SCSS block names match". Either change this to className="navigation" to match the existing SCSS, or update Navigation.scss to use .nav { &__link { ... } } so the compiled selectors match the DOM.

mateacademy-ai-mentor · 2026-07-07T13:57:13Z

+  width: 100%;
+  padding: 1em;
+  font-size: 140%;
+  position: sticky;


The <nav> element uses className="nav" while the SCSS defines .navigation as the block. This breaks the requirement to align navigation class names between JSX and SCSS and prevents the BEM block styles from applying correctly. Either change this to className="navigation" to match your SCSS, or update Navigation.scss to use .nav { &__link { ... } }.

mateacademy-ai-mentor · 2026-07-07T13:57:13Z

+// Create a Header function returning the HTML of header block
+function Nav() {
+  return (
+    <nav className="nav">


The <nav> element uses className="nav" while your SCSS defines .navigation { &__link { ... } }. This violates the requirement to align navigation class names so JSX and SCSS block names match, and prevents the nav block styles from applying. Either change this to <nav className="navigation"> or update Navigation.scss to use .nav { &__link { ... } }.

mateacademy-ai-mentor · 2026-07-07T13:57:13Z

+    outline: none;
+    text-decoration: none;
+    opacity: 0.7;
+    padding: 0 0.5em;


<nav> uses className="nav" while your SCSS defines .navigation as the block. This means the BEM block name in JSX (nav) does not match the SCSS block (.navigation), violating the requirement to "Align the navigation class names so the JSX and SCSS block names match" and preventing styles from applying as intended. Either change this to className="navigation" or update Navigation.scss to use .nav { &__link { ... } }.

mateacademy-ai-mentor · 2026-07-07T13:57:13Z

 // Create a Welcome function returning the HTML of welcome block
+function Welcome() {
+  return (
+    <section className="welcome">


The <nav> element uses className="nav" while your SCSS defines the block as .navigation { &__link { ... } }. This means the BEM block name in JSX does not match the SCSS block name, violating the requirement to align navigation class names so JSX and SCSS block names match, and the styles will not apply as intended. Either change this to <nav className="navigation"> or update Navigation.scss to use .nav { &__link { ... } }.

mateacademy-ai-mentor

Review Summary

I am approving your solution as part of an educational override so you can keep progressing while learning. You’ve done a solid job decomposing the app into Welcome, Header, Navigation, and Article components with co-located SCSS, and your class names are now consistently aligned between JSX and styles, which matches the decomposition and structure requirements (including [CHECKLIST ITEM #1] and [CHECKLIST ITEM #2]). One remaining issue to fix is in src/App.jsx: at the root of the component you no longer need className="app", and since App.css doesn’t define .app, you should either remove that class or add a matching .app rule to avoid an unused class (this was a HIGH requirement from the previous review). The best practice here is to keep JSX and CSS in sync so every className either has a clear styling purpose or is removed. As a minor process reminder for [CHECKLIST ITEM #3], run git status locally to make sure all new component and SCSS files (especially Navigation) are added and committed. Overall, your structure and use of default exports look good—just treat the remaining points as polishing steps to bring your solution fully in line with the requirements.

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Rate AI review example ↩

mateacademy-ai-mentor · 2026-07-07T17:29:46Z

+  return (
+    <header className="header">
+      <h1 className="header__title">Site Name</h1>
+      <Navigation />


You still have a root App container using className="app" in JSX while App.css doesn’t define a .app rule, which violates the previous HIGH requirement: either add a .app selector with your main layout styles or remove/rename this className to avoid having an unused class on the root element.

fix#1

83114e6

mateacademy-ai-mentor suggested changes Jul 7, 2026

View reviewed changes

mirlik added 2 commits July 7, 2026 16:53

fix#2

2a4d1e8

fix#3

af3fbe4

mirlik requested a review from mateacademy-ai-mentor July 7, 2026 13:55

mateacademy-ai-mentor suggested changes Jul 7, 2026

View reviewed changes

mirlik added 2 commits July 7, 2026 20:07

fix#4

0a43fa1

fix#5

bbbcc4a

mirlik requested a review from mateacademy-ai-mentor July 7, 2026 17:28

mateacademy-ai-mentor approved these changes Jul 7, 2026

View reviewed changes

		@@ -0,0 +1,21 @@
		// import a css file containig header styles

Uh oh!

Conversation

mirlik commented Jul 7, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants