Skip to content

solution 2#4277

Open
OliverBars wants to merge 3 commits intomate-academy:masterfrom
OliverBars:develop
Open

solution 2#4277
OliverBars wants to merge 3 commits intomate-academy:masterfrom
OliverBars:develop

Conversation

@OliverBars
Copy link

No description provided.

Copy link

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

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 are still a couple of critical requirement violations that would block the task from being considered complete.

Critical issues that must be fixed:

  1. The navigation has not been moved into its own Navigation component as required. In src/components/Header/Header.jsx, the <nav>...</nav> block is still inline; you need to create src/components/Navigation/Navigation.jsx, move that markup there, and then render <Navigation /> instead of the raw <nav> in Header (and import it where needed). Likewise, the navigation styles (e.g. .navigation__link, including :hover/:focus) currently in src/components/Header/Header.css must be moved into src/components/Navigation/Navigation.css and imported from the new Navigation component so that CSS is split per component.
  2. The Welcome component is inconsistently named, which can cause import errors. In src/components/Welcome/Welcome.jsx, rename the component function from Welkome to Welcome and update the default export to export default Welcome;. Then, in src/App.jsx, fix the import and usage so they match: import Welcome from './components/Welcome/Welcome'; and <Welcome /> in the JSX.

Minor improvements (optional once the above are fixed): you may simplify the Article export to just a single default export and clean up any leftover task instruction comments, but these are not blockers. Once you address the Navigation extraction (including CSS) and the Welcome naming/import fixes, your solution should satisfy the core requirements and align with the tests’ expectations. Keep going—your overall structure is close, and these adjustments will bring it over the finish line.


✨ 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

src/App.jsx Outdated
// Move each BEM block to a separate component (file) and import them here
import Article from "./components/Article/Article";
import Header from "./components/Header/Header";
import Welkome from "./components/Welcome/Welcome";

Choose a reason for hiding this comment

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

The import name is misspelled: Welkome. Rename to match the component/folder (Welcome) to avoid confusion and potential errors. For example: import Welcome from './components/Welcome/Welcome'

src/App.jsx Outdated
diam.
</p>
</article>
<Welkome />

Choose a reason for hiding this comment

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

You render <Welkome /> here — after fixing the import above change this to <Welcome />. Also, the app does not import or render a Navigation component. This violates the requirement: "Move the navigation part of the app into a Navigation component (you may create a folder for it and move navigation there)." Add import Navigation from './components/Navigation/Navigation' and include <Navigation /> in the JSX where the navigation should appear.

font-size: inherit;
}

.navigation__link {

Choose a reason for hiding this comment

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

This file contains navigation rules (.navigation__link) which should be in the Navigation component's CSS. This violates the requirement: "Split the CSS code as well, corresponding to the decomposed components." Move these styles into a Navigation.css inside ./components/Navigation/ and import it from the Navigation component.

Comment on lines +33 to +35
.navigation__link:hover,
.navigation__link:focus {
opacity: 1;

Choose a reason for hiding this comment

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

Hover/focus rules for .navigation__link belong with the other navigation styles (not in Header.css). Move .navigation__link:hover and .navigation__link:focus to the Navigation component's stylesheet to keep styles split per component as required.

// Create a Header function returning the HTML of header block

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

Choose a reason for hiding this comment

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

Add an import for the Navigation component here (or in the parent where you want to render it). You should create ./components/Navigation/Navigation.jsx and Navigation.css and then import it, e.g. import Navigation from './components/Navigation/Navigation'.

Comment on lines +12 to +22
<nav className="navigation">
<a className="navigation__link" href="#about">
About
</a>
<a className="navigation__link" href="#services">
Services
</a>
<a className="navigation__link" href="#contact">
Contact
</a>
</nav>

Choose a reason for hiding this comment

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

The header still contains the navigation markup (<nav>...</nav>). This violates the requirement: "Move the navigation part of the app into a Navigation component (you may create a folder for it and move navigation there)." Remove this <nav> block from Header and render the new <Navigation /> component instead. Also move navigation-related styles into Navigation.css inside the Navigation folder.


import "./Welcome.css";

const Welkome = () => (

Choose a reason for hiding this comment

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

Component function name is misspelled as Welkome. Rename the function to Welcome to match the folder/filename and the task wording (this reduces confusion and avoids import/name mismatches).


// Add a default export statement for Welcome component to use it in the other files

import "./Welcome.css";

Choose a reason for hiding this comment

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

The JSX is fine and the CSS is imported correctly — keeping Welcome.css here is appropriate for this component.

</section>
);

export default Welkome;

Choose a reason for hiding this comment

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

The default export uses the misspelled name Welkome. Change this to export default Welcome; so the exported identifier matches the component name and folder structure. Also update any files that import this component if they use the old name.

Copy link

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

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 a core requirement about splitting the navigation into its own component and stylesheet is not yet met, which will block the task from being considered complete. In src/components/Header/Header.jsx, the <nav>...</nav> markup is still inside the Header component; per the instructions, that entire <nav>...</nav> block must be moved into a new src/components/Navigation/Navigation.jsx file and rendered in Header via <Navigation /> with the appropriate import. In src/components/Header/Header.css, the navigation-related rules (e.g. the .navigation__link selector and its :hover / :focus pseudo-class rules) must be removed from this file and placed into a new src/components/Navigation/Navigation.css, which should then be imported from the new Navigation component instead of from Header. These two issues are critical because the assignment explicitly requires component-level separation for navigation markup and styles, and tests/checks are likely expecting this structure.

Other than that, your overall component structure (App, Article, Welcome, Header) and default exports look solid, and the existing CSS for non-navigation parts of Header and the other components is in good shape. Once you extract the navigation into Navigation.jsx and Navigation.css and wire it up from Header, your solution should align well with 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment on lines +12 to +22
<nav className="navigation">
<a className="navigation__link" href="#about">
About
</a>
<a className="navigation__link" href="#services">
Services
</a>
<a className="navigation__link" href="#contact">
Contact
</a>
</nav>

Choose a reason for hiding this comment

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

This file still contains the navigation markup. This violates the requirement: "Navigation markup currently inside Header (the <nav>...</nav> block) must be moved into src/components/Navigation/Navigation.jsx, and then rendered via <Navigation /> in Header (with the proper import)." Move the whole <nav>...</nav> block into a new Navigation component and render <Navigation /> here instead.


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

import "./Header.css";

Choose a reason for hiding this comment

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

Header.css is imported here, but the navigation-specific rules (for example .navigation__link and its :hover/:focus) must be moved from src/components/Header/Header.css into src/components/Navigation/Navigation.css and imported from the new Navigation component. See Header.css which currently contains .navigation__link rules .

Comment on lines +23 to +31
.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;
}

Choose a reason for hiding this comment

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

This violates the implementation requirement: "Navigation styles (e.g. .navigation__link, including :hover / :focus) currently in src/components/Header/Header.css must be moved into src/components/Navigation/Navigation.css and imported from the new Navigation component." Move these .navigation__link rules into the Navigation CSS and remove them from Header.css so styles are split per component.

Comment on lines +33 to +36
.navigation__link:hover,
.navigation__link:focus {
opacity: 1;
}

Choose a reason for hiding this comment

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

This violates the same implementation requirement for the :hover / :focus rules — those pseudo-class rules must also be moved to src/components/Navigation/Navigation.css and imported from the Navigation component.

Copy link

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

Choose a reason for hiding this comment

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

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.

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.

3 participants