Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job splitting the application into components! The project structure is much cleaner. However, a couple of changes are needed to fully meet the task requirements.
The main issue is with how components are exported and imported. The task requires using export default for all components. Currently, your components have both a named export (e.g., export const Header = () => ...) and a default export. Please remove the named exports and only use the default export at the end of each component file. Consequently, you'll need to update the import statements in src/App.jsx to use the default import syntax, like this:
import Header from './components/Header/Header';
Additionally, some component-specific styles for .header and .article remain in the global src/App.css file within a media query. Please move these styles to their corresponding component CSS files (Header.css and Article.css) to complete the style encapsulation.
Once these changes are made, your solution will be perfect. Keep up the great work!
✨ 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
src/App.jsx
Outdated
| @@ -1,86 +1,16 @@ | |||
| import React from 'react'; | |||
| import './App.css'; | |||
| import { Article } from './components/Article/Article'; | |||
There was a problem hiding this comment.
The task requires using export default for components. Since Article is exported as a default, it should be imported using the default import syntax: import Article from './components/Article/Article';.
src/App.jsx
Outdated
| import React from 'react'; | ||
| import './App.css'; | ||
| import { Article } from './components/Article/Article'; | ||
| import { Header } from './components/Header/Header'; |
There was a problem hiding this comment.
The task requires using export default for components. Since Header is exported as a default, it should be imported using the default import syntax: import Header from './components/Header/Header';.
src/App.jsx
Outdated
| import './App.css'; | ||
| import { Article } from './components/Article/Article'; | ||
| import { Header } from './components/Header/Header'; | ||
| import { Welcome } from './components/Welcome/Welcome'; |
There was a problem hiding this comment.
The task requires using export default for components. Since Welcome is exported as a default, it should be imported using the default import syntax: import Welcome from './components/Welcome/Welcome';.
src/components/Article/Article.jsx
Outdated
| import './Article.css'; | ||
|
|
||
| // Create an Article function returning the HTML of article block | ||
| export const Article = () => { |
There was a problem hiding this comment.
The task requires using export default for all components. While you have a default export at the end of the file, this named export is redundant and can lead to confusion. It's better to stick to a single export style per file. Please remove the export keyword here and rely solely on the export default Article; at the end of the file. You will also need to update the import statement in App.jsx to import Article from './components/Article/Article';.
| .navigation__link { | ||
| display: inline-block; | ||
|
|
||
| padding: 0 0.5em; | ||
|
|
||
| color: black; | ||
| text-decoration: none; | ||
|
|
||
| opacity: 0.7; | ||
| outline: none; | ||
|
|
||
| transition: opacity 0.2s ease-in-out; | ||
| } | ||
|
|
||
| .navigation__link:hover, | ||
| .navigation__link:focus { | ||
| opacity: 1; | ||
| } |
There was a problem hiding this comment.
The task description suggests creating a separate Navigation component. To keep your code modular, it would be better to move the navigation-related styles to a dedicated CSS file within a new Navigation component folder. This would fully separate the concerns of the header and the navigation.
src/components/Header/Header.jsx
Outdated
|
|
||
| import './Header.css'; | ||
| // Create a Header function returning the HTML of header block | ||
| export const Header = () => { |
There was a problem hiding this comment.
The task requires using export default for all components. You have a default export on line 24, which is correct, but this named export is unnecessary. Please remove the export keyword from this line and use only the default export. Remember to adjust the import in App.jsx accordingly.
src/components/Welcome/Welcome.jsx
Outdated
|
|
||
| import './Welcome.css'; | ||
| // Create a Welcome function returning the HTML of welcome block | ||
| export const Welcome = () => { |
There was a problem hiding this comment.
The task requires using export default for all components. While you have a default export at the end of the file, this named export is redundant. It's better to stick to a single export style per file. Please remove the export keyword here and rely solely on the export default Welcome; at the end of the file.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on refactoring the components to use default exports! The JavaScript side of this task is now perfect.
This submission requires one final change to be approved. While you correctly moved the styles for .header and .article into their component-specific CSS files, the original styles still exist inside the media query in src/App.css.
To complete the task, please remove these duplicated style blocks from src/App.css. After that, the style encapsulation will be complete. You are very close to 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,86 +1,16 @@ | |||
| import React from 'react'; | |||
| import './App.css'; | |||
There was a problem hiding this comment.
To complete the style separation, the styles for .header and .article should be removed from this global App.css file. They have been correctly moved to their component-specific stylesheets, so the duplicated styles here are no longer needed.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this submission! You've perfectly addressed the feedback from the last review. The component decomposition is now ideal, with each component having its own well-encapsulated JSX and CSS. Cleaning up the global App.css file was the final piece, and you've handled it perfectly. I am approving this solution as it meets all the project requirements. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.